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

refactor(SelectPicker): move grouped options into group element #3313

Merged

Conversation

SevenOutman
Copy link
Member

@SevenOutman SevenOutman commented Jul 27, 2023

Before: grouped option elements are siblings of group element

image

a11y inspector:
image

After: grouped option elements are within group element

image

a11y inspector:
image

@vercel
Copy link

vercel bot commented Jul 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rsuite-nextjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 21, 2023 9:57am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
rsuite-v4 ⬜️ Ignored (Inspect) Visit Preview Aug 21, 2023 9:57am

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 28, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b5a10da:

Sandbox Source
rsuite-tp-ci Configuration

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Patch coverage: 63.55% and project coverage change: -0.16% ⚠️

Comparison is base (b4cb5f9) 93.38% compared to head (b5a10da) 93.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3313      +/-   ##
==========================================
- Coverage   93.38%   93.22%   -0.16%     
==========================================
  Files         569      572       +3     
  Lines       20271    20382     +111     
  Branches     2772     2803      +31     
==========================================
+ Hits        18930    19002      +72     
- Misses        691      719      +28     
- Partials      650      661      +11     
Flag Coverage Δ
ChromeCi 93.19% <63.55%> (-0.16%) ⬇️
Firefox 92.51% <63.55%> (-0.16%) ⬇️

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

Files Changed Coverage Δ
src/Windowing/List.tsx 72.22% <ø> (ø)
src/utils/getDataGroupBy.ts 78.57% <50.00%> (-11.43%) ⬇️
src/SelectPicker/Listbox.tsx 59.55% <59.55%> (ø)
src/SelectPicker/ListboxOption.tsx 77.77% <77.77%> (ø)
src/SelectPicker/ListboxOptionGroup.tsx 80.00% <80.00%> (ø)
src/SelectPicker/SelectPicker.tsx 91.91% <83.33%> (+1.91%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SevenOutman SevenOutman marked this pull request as ready for review July 28, 2023 10:23
@SevenOutman SevenOutman changed the title wip refactor(SelectPicker): move grouped options into group element refactor(SelectPicker): move grouped options into group element Jul 28, 2023
Copy link
Member

@simonguo simonguo left a comment

Choose a reason for hiding this comment

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

The menus for SelectPicker and InputPicker are almost identical. Is it possible to make the components reusable?

src/SelectPicker/Listbox.tsx Outdated Show resolved Hide resolved
src/SelectPicker/Listbox.tsx Outdated Show resolved Hide resolved
@SevenOutman
Copy link
Member Author

The menus for SelectPicker and InputPicker are almost identical. Is it possible to make the components reusable?

Yes, the Listbox component will be reused across picker components that have similar behavior. This edit only involves SelectPicker update in order to keep changes small.

@SevenOutman SevenOutman merged commit ee103fb into main Aug 29, 2023
13 of 15 checks passed
@SevenOutman SevenOutman deleted the refactor/SelectPicker-grouped-options-element-hierarchy branch August 29, 2023 03:05
simonguo added a commit that referenced this pull request Oct 9, 2023
simonguo added a commit that referenced this pull request Oct 17, 2023
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.

None yet

2 participants