Add new no-raw-button lint rule to eslint-plugin-wonder-blocks#3054
Conversation
🦋 Changeset detectedLatest commit: 122a6e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
| }, | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Yeeting this example out of here to de-emphasize this pattern of ClickableBehavior!
|
Size Change: +657 B (+0.51%) Total Size: 131 kB 📦 View Changed
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-aegryjuaee.chromatic.com/ Chromatic results:
|
| * Custom button | ||
| * </StyledButton> | ||
| * // Applying to a WB component via style prop | ||
| * <Button style={actionStyles.inverse}> |
There was a problem hiding this comment.
I wasn't 100% sure what to do with this one, except to avoid confusion around custom components with StyledButton. It made sense to me to swap it out for a Button component. Maybe we need to rethink this utility a bit?
There was a problem hiding this comment.
I wasn't sure too, I've left some questions in the comments and tagged Juan too since he may have more context around the actionStyles!
| "skipDefaultLibCheck": true, // it's safe to assume that built-in .d.ts files are correct | ||
| "skipLibCheck": false | ||
| "moduleResolution": "node", | ||
| "noImplicitAny": true |
There was a problem hiding this comment.
Note: The IconButton in the demo for this rule required some updates to tsconfig. There was quite a bit of overlap with the common config so I opted to change it a bit.
marcysutton
force-pushed
the
WB-2340-no-raw-button
branch
from
9ca6b30 to
3542853
Compare
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (81094fa) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3054"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3054 |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3054 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Thanks for working on this Marcy! I started reviewing and left some questions I wanted to get your thoughts on first since they were following a similar theme! I'll start a thread on Slack too so we can discuss more!
| <IconButton | ||
| icon={magnifyingGlassIcon} | ||
| aria-label="Search" | ||
| onClick={() => {}} | ||
| /> |
There was a problem hiding this comment.
🎉 nice! This is the type of things we want it to catch!
| * NOTE: We recommend using existing Wonder Blocks interactive components (e.g. | ||
| * `Button`, `IconButton`, `Link` etc.). These styles are meant to be used | ||
| * as a last resort when our components do not meet your needs. | ||
| * `actionStyles` is used **internally** by Wonder Blocks components to ensure |
There was a problem hiding this comment.
I see actionStyles is used externally in frontend! And I think the actionStyles are meant to be used externally so this might be misleading! We do prefer consumers to use WB components directly though.
cc: @jandrade in case you had thoughts on this!
There was a problem hiding this comment.
That's correct, actionStyles are also used in frontend to work well over dark backgrounds, so we would need to keep this while we figure a long term solution from the Design/UX perspective.
| * Custom button | ||
| * </StyledButton> | ||
| * // Applying to a WB component via style prop | ||
| * <Button style={actionStyles.inverse}> |
There was a problem hiding this comment.
I wasn't sure too, I've left some questions in the comments and tagged Juan too since he may have more context around the actionStyles!
| <StyledButton style={focusStyles.focus}> | ||
| Custom button | ||
| </StyledButton> | ||
| <Button onClick={() => {}} style={focusStyles.focus}> |
There was a problem hiding this comment.
Hmmm I wonder if we should also leave these as is to show that focusStyles work on elements that don't already have the focus styling!
There was a problem hiding this comment.
yeah, I'd keep it this way (using the current custom element) as Button already uses focusStyles internally.
There was a problem hiding this comment.
Reset back to the way it was!
marcysutton
force-pushed
the
WB-2340-no-raw-button
branch
from
3542853 to
4144c10
Compare
marcysutton
force-pushed
the
WB-2340-no-raw-button
branch
3 times, most recently
from
1dcaae2 to
ba7211a
Compare
marcysutton
force-pushed
the
WB-2340-no-raw-button
branch
2 times, most recently
from
565967c to
9e69335
Compare
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Thanks Marcy! Left some questions for you!
| aria-disabled={ariaDisabled} | ||
| onClick={disabled ? undefined : onClick} | ||
| data-testid={testId} | ||
| style={[styles.reset, disabled && styles.disabled, style]} |
There was a problem hiding this comment.
Is it worth noting that consumers will also need to provide their own disabled styles to the custom opener too? (ie. greying out the opener using semantic color disabled tokens!)
|
|
||
| /** | ||
| * Respond to a raw "blur" event. | ||
| */ | ||
| onBlur?: (e: React.FocusEvent) => unknown; | ||
|
|
||
| /** | ||
| * Respond to a raw "focus" event. | ||
| */ | ||
| onFocus?: (e: React.FocusEvent) => unknown; |
There was a problem hiding this comment.
We can add a new changeset file for a minor change in the buttons package since button components support new props!
There was a problem hiding this comment.
Added! I replaced the original changeset with a couple of targeted ones.
There was a problem hiding this comment.
Thanks for updating the changesets! I don't see a changeset for the new button props though!
There was a problem hiding this comment.
Ah, good catch! I missed one in the swirl of changesets. Added it!
| style={[ | ||
| styles.customOpener, | ||
| focused && styles.focused, | ||
| hovered && styles.hovered, | ||
| pressed && styles.pressed, | ||
| ]} |
There was a problem hiding this comment.
Would it be helpful for the custom opener examples to show full implementations of how the custom opener should handle all states? (Disabled, hover, pressed, error)
These are the things that the default opener comes with, which is why it is preferred over custom implementations!
There was a problem hiding this comment.
Yes, added some styles back in. I put them on a styles.root object so the child BodyText can be customized too!
…ule to encourage use of WB components instead
…nt for custom Dropdowns
…rule for internal button usage
marcysutton
force-pushed
the
WB-2340-no-raw-button
branch
from
9e69335 to
bca04fc
Compare
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Looks like the CustomOpener story is rendering on 2 lines, is that expected? Might also be related to the new shift in Chromatic snapshots!
Only noticed a few more minor things:
- the missing changeset for the new button props
- enabling snapshots for all themes
After that, should be good to go!
| tags: ["!autodocs", "!manifest"], | ||
| parameters: { | ||
| chromatic: { | ||
| modes: themeModes, |
There was a problem hiding this comment.
Could you switch this to allThemeModes? That will include the new dark theme! (I will be removing themeModes soon after landing the dark mode changes!)
…seButtonProps and a related ActivityButton fix
4 participants
Summary:
Adds a new lint rule to warn about raw and StyledButton elements in favor of the WB Buttoncomponent. Typically these custom implementations are lacking design system defaults and accessibility requirements like global focus styles.
Issue: WB-2340
Test plan: