Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update Datagrid filter to account for reoccuring metadata columns #10046

Closed
wants to merge 2 commits into from

Conversation

thiagodallacqua-hpe
Copy link
Contributor

@thiagodallacqua-hpe thiagodallacqua-hpe commented Oct 11, 2024

Ticket

ET-790

Description

Right now, if we have multiple metadata columns with the same name, there's no quick way of knowing of which type each one is while selecting it. This PR intends to add a descriptive text next to duplicate entries for the metadata columns.

Test Plan

  • make sure that you have at least one metadata column that is duplicated and of a different type
  • go to a experiment list
    • make sure that the grid visualization is enabled
  • open the filter
  • scroll down for the metadata column options
  • verify that the duplicates has a <column_name> <column_type_tag> format
Screenshot 2024-10-14 at 16 58 13 Screenshot 2024-10-14 at 16 59 27 Screenshot 2024-10-18 at 20 04 36

Screenshot 2024-10-14 at 15 59 55

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 8b26336
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/671924619874f000081a9d33

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 72.91667% with 26 lines in your changes missing coverage. Please review.

Project coverage is 50.90%. Comparing base (cbf0bb1) to head (3f3ced0).

Files with missing lines Patch % Lines
...c/components/FilterForm/components/FilterField.tsx 37.03% 17 Missing ⚠️
webui/react/src/components/MultiSortMenu.tsx 86.20% 7 Missing and 1 partial ⚠️
webui/react/src/pages/FlatRuns/FlatRuns.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           thiago/ET-791   #10046      +/-   ##
=================================================
+ Coverage          50.89%   50.90%   +0.01%     
=================================================
  Files                953      953              
  Lines             130355   130426      +71     
  Branches            3653     3663      +10     
=================================================
+ Hits               66339    66394      +55     
- Misses             63882    63897      +15     
- Partials             134      135       +1     
Flag Coverage Δ
web 54.35% <72.91%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
webui/react/src/components/ColumnPickerMenu.tsx 96.01% <100.00%> (ø)
...ui/react/src/components/FilterForm/TableFilter.tsx 80.20% <100.00%> (+0.20%) ⬆️
...omponents/FilterForm/components/FilterFormStore.ts 91.45% <100.00%> (+0.02%) ⬆️
webui/react/src/utils/flatRun.ts 99.24% <100.00%> (+<0.01%) ⬆️
webui/react/src/pages/FlatRuns/FlatRuns.tsx 11.27% <0.00%> (ø)
webui/react/src/components/MultiSortMenu.tsx 88.23% <86.20%> (-0.59%) ⬇️
...c/components/FilterForm/components/FilterField.tsx 57.21% <37.03%> (-0.44%) ⬇️

@thiagodallacqua-hpe thiagodallacqua-hpe marked this pull request as draft October 11, 2024 19:01
@thiagodallacqua-hpe thiagodallacqua-hpe changed the title thiago/ET-790 feat: update Datagrid filter to account for reoccuring metadata columns Oct 11, 2024
@thiagodallacqua-hpe thiagodallacqua-hpe removed the request for review from EmilyBonar October 11, 2024 19:02
@thiagodallacqua-hpe thiagodallacqua-hpe marked this pull request as ready for review October 14, 2024 19:23
@thiagodallacqua-hpe thiagodallacqua-hpe requested a review from a team as a code owner October 14, 2024 19:23
Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we want to show types in all column names

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you double-check the ticket? i think specs around separator etc are not follwoing the ticket description.

webui/react/src/services/types.ts Outdated Show resolved Hide resolved
webui/react/src/components/TableActionBar.tsx Outdated Show resolved Hide resolved
@thiagodallacqua-hpe
Copy link
Contributor Author

@keita-determined it turns out that array-typed columns are banned from the filters

const bannedFilterColumns: Set<string> = useMemo(() => {
    return new Set([...BANNED_FILTER_COLUMNS, ...arrayTypeColumns]);
  }, [arrayTypeColumns]);

i know its banned. but when there are metadata with the same name and different type, the metadata won't display in the filter.

that's fixed 😅

Screenshot 2024-10-24 at 17 33 35

Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update the table filter tests -- it seems that they check the label text and now it's failing.

feat: add metadata column type badge

chore: lint
chore: rebase
Update webui/react/src/components/FilterForm/components/FilterField.tsx

Co-authored-by: Ashton G. <[email protected]>
chore: update test
@thiagodallacqua-hpe
Copy link
Contributor Author

Closing PR due to company restructuring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants