-
Notifications
You must be signed in to change notification settings - Fork 639
Fix MarkdownEditor focus variable usage and border bottom missing #3911
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
Conversation
|
size-limit report 📦
|
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.
Left an optional improvement, approving in advance
Co-authored-by: Siddharth Kshetrapal <[email protected]>
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.
Sweet - I'd love to cut a release after we merge this so we can ship these updates in dotcom.
| '&: focus-within': | ||
| view === 'edit' | ||
| ? { | ||
| outline: '2px solid var(--borderColor-accent-emphasis)', | ||
| outline: '2px solid', | ||
| outlineColor: 'accent.emphasis', | ||
| } | ||
| : {}, |
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.
I think this is non-blocking since it feels like something we can tweak later, but the PVC component only shows focus on the whole editor when the textarea is focused. When focus is on the toolbar, the blue outline around the editor is not rendered. For example:
The same is not true for this component, where we show the outline whenever focus is anywhere inside the editor:
The problem is how we would do this: has would be the perfect solution here (&:has(textarea:focus), but browser support is not there (thanks Firefox). Maybe it's worth waiting for browser support rather than rolling some custom React state-based solution.
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.
Should we capture this in a new issue? It seems like Firefox will support :has by Christmas: https://groups.google.com/a/mozilla.org/g/dev-platform/c/oacuvZ2_hLg/m/4o28pFLkAwAJ?pli=1 🎉
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.
Oh that's interesting. We already use :has in primer/react in a few places. I'm assuming this is because dotcom uses a polyfill
Fixes an issue with MarkdownEditor's focus token usage that wasn't picked up causing focus to not be visible part of #3901 as well as an issue that caused the borderBottom for the Preview tab to not be visible.
Before:
CleanShot.2023-11-06.at.10.20.19.mp4
##After:
CleanShot.2023-11-06.at.10.20.42.mp4
Changelog
Changed
themefunction, as the variable itself seems like it's no longer availableborderBottomRollout strategy
Testing & Reviewing
Head to http://127.0.0.1:6006/?path=/story/drafts-components-markdowneditor--playground and focus the text area or navigate to the Preview tab
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.