[WB-1868.1] Dropdown: use Cell default styles #2659
Conversation
🦋 Changeset detectedLatest commit: ee7fb8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
Size Change: -432 B (-0.43%) Total Size: 100 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-bedjtccptr.chromatic.com/ Chromatic results:
|
…ickable cells can't be focusable via keyboard navigation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (e1499c9) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2659"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2659 |
| onMouseDown={(e) => { | ||
| // Prevents the combobox from losing focus when clicking | ||
| // on the option item. | ||
| e.preventDefault(); | ||
| }} |
There was a problem hiding this comment.
Is the equivalent logic needed for keyboard interactions?
There was a problem hiding this comment.
Yes, I just moved that from option-item to this component directly to simplify the implementation (forgot to add a note here).
| @@ -8,16 +8,12 @@ import { | |||
| border, | |||
| sizing, | |||
| } from "@khanacademy/wonder-blocks-tokens"; | |||
| import {LabelMedium, LabelSmall} from "@khanacademy/wonder-blocks-typography"; | |||
| import {LabelMedium} from "@khanacademy/wonder-blocks-typography"; | |||
There was a problem hiding this comment.
Could we use the BodyText component? (not sure if you had plans for that later!)
There was a problem hiding this comment.
good question! I had plans to add it in the next PR as this one was already including a bunch of changes, but I can add it here if you prefer.
There was a problem hiding this comment.
In another PR is fine too :)
| @@ -143,8 +139,6 @@ type DefaultProps = { | |||
| selected: OptionProps["selected"]; | |||
| }; | |||
|
|
|||
| const StyledLi = addStyle("li"); | |||
There was a problem hiding this comment.
Did we intend to remove using the li semantics for combobox option items?
There was a problem hiding this comment.
Yeah! I saw that it had this structure:
listbox
> li
> il
I changed it to use the correct semantics:
listbox
> option
> option
Let me know if you see something off.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/tb-cell-dd #2659 +/- ##
==========================================
==========================================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
## Summary: This PR includes the following commits: Issue: WB-XXXX ## Test plan: - [feature/tb-cell-dd] Trigger empty commit for creating integration branch PR - [🔥AUDIT🔥] [WB-1958.1] Refactor Cell stories to use StateSheet (#2643) - [WB-1958.2] Add theming support to Cell (#2644) - [WB-1959] Add thunderblocks theme to CompactCell and DetailCell (#2657) - [WB-1959.3] Cell layout refactor (#2661) - [WB-1868.1] Dropdown: use Cell default styles (#2659) - [WB-1868.2] Dropdown: Theming set up (#2675) - [WB-1868.3] Add ThunderBlocks theme to dropdown (#2680) - [feature/tb-cell-dd] Fix merge conflicts [WB-1959]: https://khanacademy.atlassian.net/browse/WB-1959?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Author: jandrade Reviewers: beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ 19 checks were successful, ⏹️ 4 checks were cancelled, ⏭️ 3 checks have been skipped Pull Request URL: #2642
Summary:
This is a prep PR to simplify some of the dropdown styles before adding theming support to it.
OptionItemandActionItemstyles to just use the default CellCore styles.OptionItemstructure so the same layout can be reused bySingle/MultiSelectandListbox.tabIndexprop toCellso the component can work with different focus modes (needed for visual focus on Listbox/Combobox).blueinstead ofoffBlack.Implementation plan:
Issue: https://khanacademy.atlassian.net/browse/WB-1868
Test plan:
Navigate to the Dropdown docs and verify that the stories look correct.