-
Notifications
You must be signed in to change notification settings - Fork 354
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
fix: improve sorting for multi-type columns #10123
Conversation
32f655a
to
933c480
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bug where the column types are being grouped by the name, and, same as the bug that I solved in the #10046 the bannedColumns
are including the duplicate of the array type. Perhaps you can rebase from that branch to also the fix.
this improves the experience for interacting with multi-type columns: * as sorting is type-agnostic, the multi-sort menu will show all affected types as badges next to the column name * all columns with the same key will appear as sorted in the header * multisort menu copy is updated to use the unspecified copy for these columns, instead of specific copy (but header copy for sort menu items remains the same).
177b997
to
aff0f22
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## thiago/ET-790 #10123 +/- ##
================================================
Coverage ? 53.82%
================================================
Files ? 447
Lines ? 77383
Branches ? 3656
================================================
Hits ? 41648
Misses ? 35600
Partials ? 135
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@thiagodallacqua-hpe it went away with the rebase as the multi-sort column select uses the same banned columns property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor FYI, other than that LGTM
const badges = typeMap[c.column].map((type) => { | ||
const copy = | ||
(runColumns as readonly string[]).includes(c.column) && | ||
type === V1ColumnType.UNSPECIFIED | ||
? 'BOOLEAN' | ||
: removeColumnTypePrefix(type); | ||
return ( | ||
<Fragment key={type}> | ||
<Badge text={copy} />{' '} | ||
</Fragment> | ||
); | ||
}); | ||
const label = ( | ||
<> | ||
{c.displayName || c.column} {badges} | ||
</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh... idk if it's better to show the badges grouped or individually, for instance, the first time that I saw it I thought that it was a bug 😅... maybe that's something to keep in mind when we add documentation for the release
Ticket
ET-836
Description
this improves the experience for interacting with multi-type columns:
as sorting is type-agnostic, the multi-sort menu will show all affected types as badges next to the column name
all columns with the same key will appear as sorted in the header
multisort menu copy is updated to use the unspecified copy for these columns, instead of specific copy (but header copy for sort menu items remains the same).
Test Plan
with a project with multi-type metadata columns, visit the project runs page.
Checklist
docs/release-notes/
See Release Note for details.