fix(SelectPanel): selected items should appear at the top #5867
Conversation
🦋 Changeset detectedLatest commit: 334664d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
github-actions
bot
added
the
integration-tests: recommended
| const itemASelected = selectedOnSort.some(item => | ||
| Object.entries(item).every(([key, value]) => { | ||
| if (key === 'selected') { | ||
| return true | ||
| } | ||
| return itemA[key as keyof ItemProps] === value | ||
| }), | ||
| ) |
There was a problem hiding this comment.
this seems expensive :(
There was a problem hiding this comment.
Wanted to ask if it would be possible for the sort order to be maintained as a stateful value and compute it onClose or onOpen instead of having to run the sort every render?
It seemed like the logic that we have for this is something like:
- When open, keep the ordering of items constant
- When closing (or when re-opening), sort the items by:
- selected items come first
- sort selected items by group, if applicable
- otherwise, sort by alphanumeric
- otherwise, sort by alphanumeric
- selected items come first
Does that track with what you're seeing?
| // Then order by text | ||
| if ((itemA.text ?? '') < (itemB.text ?? '')) { | ||
| return -1 | ||
| } else if ((itemA.text ?? '') > (itemB.text ?? '')) { | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
I baked in alphabetically ordering through text but now that I'm looking at the issue it doesn't really look like a requirement 🤔
I can remove and do selection-at-top only if this is not wanted/needed 👀
CC: @emilybrick
There was a problem hiding this comment.
Consumers could want to sub-sort based on something besides alphabetical order. For example: sort most frequently used labels to the top.
I think we should remove this default alphanumeric sort.
There was a problem hiding this comment.
Feel free to steal the alphanumeric stuff from data table btw if it's helpful! We have some sort helpers over there
size-limit report 📦
|
| }, | ||
| } as ItemProps | ||
| }) | ||
| .sort((itemA, itemB) => { |
There was a problem hiding this comment.
I'm coding this as the default behavior. Wondering if we should allow consumer to opt-out of this behavior or provide their custom sort function 👀
cc: @emilybrick
There was a problem hiding this comment.
I feel strongly that we should let consumers opt-out by providing their own sort function. I think DataTable has a nice API for handling this: https://github.com/primer/react/blob/main/packages/react/src/DataTable/column.ts#L59
…nto fix/select-panel-ordering
There was a problem hiding this comment.
visual updates are due to items being sorted now
francinelucca
requested review from
mperrotti,
emilybrick,
hectahertz and
joshblack
|
🔴 golden-jobs completed with status |
francinelucca
assigned emilybrick, francinelucca and hectahertz and unassigned emilybrick and francinelucca
|
This looks good visually - though I think we should remove the border that appears under the selected items. The items being selected differentiates them enough from the normal list. |
|
Uh oh! @emilybrick, at least one image you shared is missing helpful alt text. Check #5867 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
| }, | ||
| } as ItemProps | ||
| }) | ||
| .sort((itemA, itemB) => { |
There was a problem hiding this comment.
I feel strongly that we should let consumers opt-out by providing their own sort function. I think DataTable has a nice API for handling this: https://github.com/primer/react/blob/main/packages/react/src/DataTable/column.ts#L59
| // Then order by text | ||
| if ((itemA.text ?? '') < (itemB.text ?? '')) { | ||
| return -1 | ||
| } else if ((itemA.text ?? '') > (itemB.text ?? '')) { | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
Consumers could want to sub-sort based on something besides alphabetical order. For example: sort most frequently used labels to the top.
I think we should remove this default alphanumeric sort.
| @@ -213,6 +213,22 @@ | |||
| } | |||
| } | |||
|
|
|||
| &:where([data-last-selected]:not(:last-of-type)) { | |||
There was a problem hiding this comment.
I like this CSS-only solution for visually separating selected and unselected items, but where did this design change come from? We didn't do this before.
There was a problem hiding this comment.
here's the issue https://github.com/github/primer/issues/2409!
| position: absolute; | ||
| bottom: 0; | ||
| width: calc(100% + var(--base-size-16)); | ||
| margin-left: calc(-1 * var(--base-size-8)); | ||
| content: ''; | ||
| /* stylelint-disable-next-line primer/borders */ | ||
| border-bottom: 1px solid var(--borderColor-default); | ||
| border-bottom-right-radius: 0; | ||
| border-bottom-left-radius: 0; |
There was a problem hiding this comment.
A shorter way to do this would be:
| position: absolute; | |
| bottom: 0; | |
| width: calc(100% + var(--base-size-16)); | |
| margin-left: calc(-1 * var(--base-size-8)); | |
| content: ''; | |
| /* stylelint-disable-next-line primer/borders */ | |
| border-bottom: 1px solid var(--borderColor-default); | |
| border-bottom-right-radius: 0; | |
| border-bottom-left-radius: 0; | |
| position: absolute; | |
| content: ''; | |
| inset: 100% calc(-1 * var(--base-size-8)); | |
| background-color: var(--borderColor-default); | |
| height: var(--borderWidth-default); |
| @@ -213,6 +213,22 @@ | |||
| } | |||
| } | |||
|
|
|||
| &:where([data-last-selected]:not(:last-of-type)) { | |||
| padding-bottom: var(--base-size-4); | |||
There was a problem hiding this comment.
Why do we reduce the padding for these items?
There was a problem hiding this comment.
I'm actually adding padding to separate from the divider line, this is only added to the last selected item for such purposes
| } | ||
| }, [selected, setSelectedOnSort]) | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Some of these places where we're using an effect to reset the state might not be needed, this section from the docs came to mind: https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
Maybe for some of them we could run after a user action (like when the overlay is closed / opened or when onFilterValue is changed so that filterValue is empty)
| @@ -324,40 +336,146 @@ export function SelectPanel({ | |||
| } | |||
| }, [placeholder, renderAnchor, selected]) | |||
|
|
|||
| const resetSort = useCallback(() => { | |||
There was a problem hiding this comment.
I think if we're not passing this as an argument to the effects below then we should not use useCallback here
| const itemASelected = selectedOnSort.some(item => | ||
| Object.entries(item).every(([key, value]) => { | ||
| if (key === 'selected') { | ||
| return true | ||
| } | ||
| return itemA[key as keyof ItemProps] === value | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Wanted to ask if it would be possible for the sort order to be maintained as a stateful value and compute it onClose or onOpen instead of having to run the sort every render?
It seemed like the logic that we have for this is something like:
- When open, keep the ordering of items constant
- When closing (or when re-opening), sort the items by:
- selected items come first
- sort selected items by group, if applicable
- otherwise, sort by alphanumeric
- otherwise, sort by alphanumeric
- selected items come first
Does that track with what you're seeing?
| // Then order by text | ||
| if ((itemA.text ?? '') < (itemB.text ?? '')) { | ||
| return -1 | ||
| } else if ((itemA.text ?? '') > (itemB.text ?? '')) { | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
Feel free to steal the alphanumeric stuff from data table btw if it's helpful! We have some sort helpers over there
A big one is we want to reset the sorting when the filter value is cleared, we also do want to sort on the first open, so I don't think it's as simple as onClose/onOpen. There's definitely some cleanup we might be able to do if we keep a state of the mapped items though 👀 |
|
@emilybrick double-checking, you're saying remove the line separation entirely, just keep the ordering on top functionality? |
Closes https://github.com/github/primer/issues/2409
Adds logic to SelectPanel to automatically sort items, displaying the selected items first and then sorting alphabetically. The sorted list gets rearranged on:
SingleSelect
Before
After
MultiSelect
Before
After
With Dividers
Before
After
Full Screen
Before
After
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
SelectPanel stories
Merge checklist