[DCJ-741] refactor dataset table into two dataset and studies table #2691

s-rubenstein · 2024-10-17T19:18:37Z

Addresses

Splits the collapsing table into two tables, studies and datasets

Summary

I added typing to the table, and abstracted it out into its own components
I added optional tooltips if the text overflowed, rather than expanding the rows - this reduces screen jitter.

Screen.Recording.2024-10-16.at.4.15.47.PM.mov

Have you read Terra's Contributing Guide lately? If not, do that first.

Label PR with a Jira ticket number and include a link to the ticket
Label PR with a security risk modifier [no, low, medium, high]
PR describes scope of changes
Get a minimum of one thumbs worth of review, preferably two if enough team members are available
Get PO sign-off for all non-trivial UI or workflow changes
Verify all tests go green
Test this change deployed correctly and works on dev environment after deployment

s-rubenstein · 2024-10-17T19:24:28Z

src/components/SimpleTable.jsx

@@ -104,7 +104,7 @@ const DataRows = ({rowData, baseStyle, columnHeaders, rowWrapper = ({renderedRow
          let output;
          //columnHeaders determine width of the columns,
          //therefore extract width from columnHeader and apply to cell style
-          const columnWidthStyle = columnHeaders[cellIndex].cellStyle;
+          const columnWidthStyle = { width: columnHeaders[cellIndex].cellStyle.width };


I didn't know where to check where this was used - but I was noticing we said we were extracting just the width, but actually pulling everything. I figured this would be cleaner.

s-rubenstein · 2024-10-17T19:26:53Z

src/components/data_search/DatasetSearchTable.jsx

    history.push(`/dar_application/${darDraft.referenceId}`);
  };

  const clearSearchRef = () => {
    searchRef.current.value = '';
    filterHandler(null, datasets, '', '');
  };
-
-  const openTranslatedDUL = (dataUse) => {


This was unused in the new version - I figure we can add it back if need be. Not sure because I didn't see dataUse in the mocks: https://www.figma.com/design/MkffpVbUdP4lWvkN9OWiro/Data-Library?node-id=23-43&node-type=frame

Agreed - I'm usually in favor of aggressive pruning, it's easy enough to re-add. Especially for anything related to data use translations, we have some very inefficient code related to that functionality. If we do need it, we need to re-evaluate how we do it.

rjohanek · 2024-10-17T20:05:29Z

src/components/data_search/DatasetSearchTable.jsx

@@ -463,14 +248,6 @@ export const DatasetSearchTable = (props) => {
          }
        </Box>
      </Box>
-      {
-        showTranslatedDULModal &&
-        <TranslatedDulModal


why did this go away?

It wasn't used - I don't know if the new mocks have it or not, but I figure we can always readd it if need be

rjohanek · 2024-10-17T20:09:04Z

src/types/model.ts

+  updateUser: UserTerm;
+  dac: DacTerm;
+}
+


I'd love to hear more about the design of your approach. I'm wondering about the distinctions between this file and the constants file. Additionally, I'm wondering about the abstraction to the Display file.

overall, this looks great and the video looks good, just looking for a bit more info for my understanding!

@rjohanek great question! The goal was to break up and separate concerns. Previously, the DatasetSearchTable handled a lot - I wanted to reduce what it handled. The first abstraction was to pull out the table. Eventually I thought pulling out the constants would be nice as well, so DatasetSearchTableDisplay just handled the rendering of the table, and DatasetSearchTableConstants handled the constant values describing the rendering.

model.ts is specifically the type definitions of the API responses, and doesn't include any scope beyond that.

src/components/data_update/DatasetUpdate.jsx

s-rubenstein · 2024-10-18T18:05:54Z

Got it. I'll merge it in!

s-rubenstein · 2024-10-18T18:10:14Z

Screen.Recording.2024-10-18.at.2.08.46.PM.mov

rushtong

There are some really nice UX improvements here. We need to circle back and figure out where the export to TDR functionality fits now - I don't think it's in the mocks, but it is in the original version of the data library. I'm open to re-implementation in a different way if we can make that a priority.

src/components/Tooltips.tsx

rushtong · 2024-10-20T15:38:34Z

src/components/data_search/DatasetSearchTable.jsx

    history.push(`/dar_application/${darDraft.referenceId}`);
  };

  const clearSearchRef = () => {
    searchRef.current.value = '';
    filterHandler(null, datasets, '', '');
  };
-
-  const openTranslatedDUL = (dataUse) => {


Agreed - I'm usually in favor of aggressive pruning, it's easy enough to re-add. Especially for anything related to data use translations, we have some very inefficient code related to that functionality. If we do need it, we need to re-evaluate how we do it.

src/components/data_search/DatasetSearchTable.jsx

fboulnois

After all the improvements this looks great! Thank you 👍

src/components/data_update/DatasetUpdate.jsx

rjohanek

looks good! thanks for the changes and explanations

s-rubenstein added 3 commits October 11, 2024 11:20

WIP table abstraction

6b19b2c

Table refactor

d7a3349

Fix ids

fcd5c58

s-rubenstein requested a review from a team as a code owner October 17, 2024 19:18

s-rubenstein commented Oct 17, 2024

View reviewed changes

rjohanek reviewed Oct 17, 2024

View reviewed changes

src/components/data_update/DatasetUpdate.jsx Outdated Show resolved Hide resolved

Revert unintended changes

8737914

This comment was marked as resolved.

Sign in to view

s-rubenstein added 2 commits October 18, 2024 14:07

Fix selected tab display logic

86d09e0

Merge branch 'develop' into sr/dcj-741-refactor-dataset-table

8357d4f

rushtong reviewed Oct 20, 2024

View reviewed changes

Add Export to Terra Column

02124fe

fboulnois approved these changes Oct 22, 2024

View reviewed changes

src/components/data_update/DatasetUpdate.jsx Outdated Show resolved Hide resolved

rjohanek approved these changes Oct 22, 2024

View reviewed changes

s-rubenstein merged commit 746abfc into develop Oct 22, 2024
9 checks passed

s-rubenstein deleted the sr/dcj-741-refactor-dataset-table branch October 22, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DCJ-741] refactor dataset table into two dataset and studies table #2691

[DCJ-741] refactor dataset table into two dataset and studies table #2691

s-rubenstein commented Oct 17, 2024

s-rubenstein Oct 17, 2024

s-rubenstein Oct 17, 2024

rushtong Oct 20, 2024

rjohanek Oct 17, 2024

s-rubenstein Oct 18, 2024

rjohanek Oct 17, 2024

rjohanek Oct 17, 2024

s-rubenstein Oct 18, 2024

This comment was marked as resolved.

This comment was marked as resolved.

s-rubenstein commented Oct 18, 2024

s-rubenstein commented Oct 18, 2024

rushtong left a comment

rushtong Oct 20, 2024

fboulnois left a comment

rjohanek left a comment •

edited

Loading

[DCJ-741] refactor dataset table into two dataset and studies table #2691

[DCJ-741] refactor dataset table into two dataset and studies table #2691

Conversation

s-rubenstein commented Oct 17, 2024

Addresses

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

s-rubenstein commented Oct 18, 2024

s-rubenstein commented Oct 18, 2024

rushtong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fboulnois left a comment

Choose a reason for hiding this comment

rjohanek left a comment • edited Loading

Choose a reason for hiding this comment

rjohanek left a comment •

edited

Loading