Components: Add hooks based VisuallyHidden #28887
Conversation
sarayourfriend
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
I couldn't find CSS styles for the focused state of the Without those styles, skip links won't work properly. |
|
@gziolo Thanks for pointing that out, they're indeed missing, it'll be trivial to add them using the |
|
I repeat myself, but those snapshot tests aren't very helpful. Those diffs are so gigantic that it's impossible to validate the change if you don't know the context. Therefore they don't bring too much value to the review process. We should still use them but find a better way to distill what's saved to only the bits that are important for the individual test. |
sarayourfriend
force-pushed
the
try/replace-visually-hidden
branch
from
c70a808
to
43448e5
Compare
|
@ItsJonQ Do you have any ideas what to do about the snapshot tests? I'll do a little research today to see how others have solved this issue but so far all I've seen is advise to use the emotion serializer (which we do). Update: maybe it'd be worth trying out |
sarayourfriend
changed the title
sarayourfriend
changed the title
|
Diffing between the most basic version would be a great start. I found this article where it's recommended: https://kentcdodds.com/blog/effective-snapshot-testing |
@saramarcondes Not for the time being :(. Other than to skim them to ensure they look correct, and update if necessary (what we're doing now). This is of course more difficult if lower level changes affect many other snapshots. As @saramarcondes had suggested (and Kent had mentioned in his blog post), maybe Taking a step back, replacing the current This Having chatted with @saramarcondes , I think this would be a good candidate to increase G2 Component exposure while being mindful of risk. In theory, For that, I'm feeling okay moving forward with this. @gziolo Love to hear your thoughts :) |
|
I think I’ve got something close to working with snapshot diff: Only problem is snapshot diff does not play nicely with emotion (like at all) so I think we’ll need to write a custom matcher for it. The code for the test above is this: Where The custom matcher could be named something like In any case, progress is being made here, just wanted to let y'all know. I should have a PR open soon with a proposal for this, either today or tomorrow. |
|
|
Here y'all go: #28897 |
|
I would prefer we don't remove old code until there is an official proposal that is accepted. It should be a small change here. |
|
Sounds good, we'll just move this into the |
sarayourfriend
force-pushed
the
try/replace-visually-hidden
branch
from
43448e5
to
6d781ae
Compare
|
Size Change: +2.7 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
sarayourfriend
changed the title
| export function useVisuallyHidden( { className, ...props } ) { | ||
| // circumvent the context system and write the classnames ourselves | ||
| const classes = cx( | ||
| 'components-visually-hidden wp-components-visually-hidden', |
There was a problem hiding this comment.
Out of curiosity, why does it have two similar class names?
The old implementation uses only components-visually-hidden.
There was a problem hiding this comment.
G2 introduces the wp- prefixed classname. Typically they'd be added by useContextSystem here: https://github.com/ItsJonQ/g2/blob/5feb4f2822efcd67afbf3f52849c78b8cbfd0f82/packages/context/src/use-context-system.js#L95
For consistency's sake I added both to the hooks version.
|
Some things I observe here and in general. Those are nitpicking to consider but they might help with further work:
Should we connect the new component using gutenberg/packages/components/src/font-size-picker/next.js Lines 24 to 26 in 6999bf8 or do you think it isn't worth it? |
I don't think this is necessary considering there is no need for a props adapter, we can just swap out the references directly, right? Or should we do it so that we can swap them using the context system globally? |
sarayourfriend
force-pushed
the
try/replace-visually-hidden
branch
from
05d8a01
to
6c27ae8
Compare
I'm happy with anything. For context, I'm personally use to naming conventions I have in the The only thing the proposed naming system doesn't address is sub-components 🤔 (Let's pretend there's a
(I think
@gziolo Since we're adding this (new) |
|
It's a tricky part to name files. We were using mostly |
|
Any idea where this error comes from? Would rebase help? Otherwise, it's ✅ from me. |
sarayourfriend
force-pushed
the
try/replace-visually-hidden
branch
from
5e56a6b
to
4f0800b
Compare
Description
Create a new
VisuallyHiddenbased on the existingVisuallyHidden's styles but usecreateComponentand a newuseVisuallyHiddenhook.How has this been tested?
Snapshot tests updated and tested in storybook.
Screenshots
Focused
Normal
Types of changes
New feature.
Checklist: