Add new VisuallyHidden component
#74189
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +223 B (+0.01%) Total Size: 2.6 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 084e0dd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20473483807
|
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Nice work @mirka 👍
This is looking pretty good and testing well for me.
If I understand correctly, we're leveraging BaseUI to support a render prop across all components in the @wordpress/ui package? If that's correct, this PR successfully establishes that pattern.
I've left one inline comment where I think the styles might have a typo.
The only other suggestion I might offer would be to add a test for the render prop functionality given we're establishing that as a pattern to follow in the future.
Most other components will support the render prop out-of-the-box from the Base UI component, but yes let's add a test for this one since it's integrated in by us (6c760d5). |
There was a problem hiding this comment.
Test Report
Environment
- WordPress: 7.0-alpha-61411
- PHP: 8.3.28-dev
- Server: PHP.wasm
- Database: WP_SQLite_Driver (Server: 8.0.38 / Client: 3.51.0)
- Browser: Chrome 143.0.0.0
- OS: Windows 10/11
- Theme: Twenty Twenty-Five 1.4
- MU Plugins: None activated
- Plugins:
- Gutenberg 22.3.0
- Gutenberg UI Playground 1.0.0
- Test Reports 1.2.1
Testing environment
Actual Results
- ✅ Overall looking good.
Additional Notes
- I was wondering why it was required this PR for the
Fieldone, but now I see that this PR integrates all thebase-uilibraries. Maybe it would have been better, to add 3 PR: one specific to introduce the packaging and two independent ones for each of the new components. - One extra item that tests the purpose of the component, that content is rendered but visually hidden
Supplemental Artifacts
| @layer wp-ui-components { | ||
| .visually-hidden { | ||
| border: 0; | ||
| clip: rect(1px, 1px, 1px, 1px); |
There was a problem hiding this comment.
In modern browsers, I believe it is.
I'm assuming this was a legacy fallback for IE and other older browser versions. Given we've dropped official support for IE, I suspect this would be ok to be removed.
I don't hold a strong opinion on this though as it is a very cheap fallback for additional robustness 🤷
There was a problem hiding this comment.
It seems like there are many slightly different approaches to this CSS snippet. I also don't have a strong opinion, although I'll point out that CSS Tricks and Josh Comeau recommend the same snippet:
.visually-hidden:not(:focus):not(:active) {
clip: rect(0 0 0 0);
clip-path: inset(50%);
height: 1px;
overflow: hidden;
position: absolute;
white-space: nowrap;
width: 1px;
}Other sources of inspiration are tailwind and the a11y collective
There was a problem hiding this comment.
Good point, I think we can remove clip. These incantations tend to be passed down verbatim forever 😇 552561e
| screen.getByRole( 'textbox', { name: 'My label' } ) | ||
| ).toBeVisible(); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
| } ); | |
| } ); | |
| it( 'renders content for screen readers', () => { | |
| render( <VisuallyHidden>Hidden text</VisuallyHidden> ); | |
| expect( screen.getByText( 'Hidden text' ) ).toBeInTheDocument(); | |
| } ); |
There was a problem hiding this comment.
Slightly redundant (since we're already using getBy* methods above), but it may not hurt to have an explicit test that queries the component's content.
There was a problem hiding this comment.
I thought about this, but I don't think it would be a very meaningful test for this specific case. This aspect of the component is unlikely to regress over time, and even if it did, this test would not give us much confidence. I can add display: none to the CSS, and the test would still pass.
But what's more problematic is that CSS modules are completely mocked out in this test setup, and there seem to be no sensible ways around it! So even a toBeVisible() is no longer reliable when CSS modules are involved.
This is actually a good heads-up as we expand our CSS module usage. And maybe another nudge to try Vitest browser mode. (cc @WordPress/gutenberg-components)
There was a problem hiding this comment.
Ouch. Yes, one more reason to try Vitest browser mode — given the small package size, it feels like a great time to experiment with it (it may even be a much lower entry barrier into a VizReg setup)
|
Hey @SirLouen , thank you for leaving a review! Are there any reasons why you left a blocking PR review? Reading your comments, I couldn't spot any particularly concerning aspects. Given that both @aaronrobertshaw and I took a look and didn't spot any major issues, I'm going to dismiss the blocking review, in order to move the work forward when necessary. Of course, feel free to reply and leave more feedback 🙏 |
ciampo
left a comment
There was a problem hiding this comment.
Left some comments, but overall looking good.
IMO good to merge when feedback is addressed
| @layer wp-ui-utilities, wp-ui-components, wp-ui-compositions, wp-ui-overrides; | ||
|
|
||
| @layer wp-ui-components { | ||
| .visually-hidden { |
There was a problem hiding this comment.
I've seen example of a selector including
| .visually-hidden { | |
| .visually-hidden.not(:focus):not(:active) |
Should we consider the amend?
There was a problem hiding this comment.
Interesting. Initially I thought it was for skip links, but it's more of a defensive trick for misuse? I could go either way on this, but I'm leaning a bit towards not adding it, because I did an audit of the codebase right now and I didn't find any instances of this type of misuse. I wouldn't object if you want to try it though.
There was a problem hiding this comment.
I also interpreted it as something useful for skip links (or any visually hidden, tabbable/focusable element) — and so does the a11y collective article and josh comeau.
I thought it would be a valuable feature, but no strong opinions in case you prefer to keep the current selector
There was a problem hiding this comment.
Let's keep this in our back pocket for when we discover a use case 👍
| screen.getByRole( 'textbox', { name: 'My label' } ) | ||
| ).toBeVisible(); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Slightly redundant (since we're already using getBy* methods above), but it may not hurt to have an explicit test that queries the component's content.
| @layer wp-ui-components { | ||
| .visually-hidden { | ||
| border: 0; | ||
| clip: rect(1px, 1px, 1px, 1px); |
There was a problem hiding this comment.
It seems like there are many slightly different approaches to this CSS snippet. I also don't have a strong opinion, although I'll point out that CSS Tricks and Josh Comeau recommend the same snippet:
.visually-hidden:not(:focus):not(:active) {
clip: rect(0 0 0 0);
clip-path: inset(50%);
height: 1px;
overflow: hidden;
position: absolute;
white-space: nowrap;
width: 1px;
}Other sources of inspiration are tailwind and the a11y collective
I've added some unit tests to be added, this is why its "blocking" like you call it (for me is called, requesting changes 🙀) |
Thank you for the clarification. The "Request changes" option implies feedback that must be addressed before the PR can be merged. As such, we use this option only when we want to give a clear and strong indication that the PR shouldn't be merged in its current state. For any other feedback, we usually choose the "Comment" option. I hope this gives a bit more context into the way we usually collaborate :) |
Sure, I know this is my pinpoint that the proposed tests should be merged (or opposed). |
|
I know this is getting into semantics and processes a little on a PR, so I'll keep my 2 cents short.
I appreciate the desire and need for tests; however, I don't think this sort of thing should be blocking. These additional tests could easily be added via a quick follow-up PR, should the consensus be that they do add value. |
Even for a critical blocking bug, tests covering the problem should go first! It's easy to Admittedly, it's legit to override blocks if there is some sort of urgency, and the blocker doesn't answer in a reasonable delay (depending on the degree of urgency). But here, for example, there is no urgency. Tests could be merged or opposed, and the review unblocked. And good to go 👍 In fact, technically approving this means that you are opposed to the test? |
|
@SirLouen If you observe the PRs in this repo, you'll notice that "request changes" are reserved for the most critical blockers and used sparingly, when we can't afford any ambiguity about how critical the problem is. For any other feedback, it's usually more precise and efficient to use words to express how strongly we may feel about them ("how about we do…", "what do you think about…", "we need to X because Y", etc). At least, that is how everyone has been operating in this repo. Using a blocking review feature liberally in an environment where it's typically used as a high severity signal is going to be seen as disruptive, whether or not we have the ability to dismiss them at will. Your PR reviews have been really helpful, so I'd appreciate if you could recalibrate the usage of the feature so we can all collaborate more effectively 🙏 |
Ok, I got you. This reminds me Core back in 2010-2015, with this guy from GSOC program merging 1K+ lines every other day 😅 (in the era when they were implementing all the AJAX in the backend). Now Core has become way more conservative, and since I've been around Core for a while now, I'm a little too used to this conservative mode, ngl. Knowing this, I will be a little more openhearted in this regard. |
|
|
||
| const meta: Meta< typeof VisuallyHidden > = { | ||
| title: 'Design System/Components/VisuallyHidden', | ||
| component: VisuallyHidden, |
There was a problem hiding this comment.
I just noticed we hadn't included the experimental tag we have in the other components. I wonder if we want to be consistent here, or it should be obvious enough by the fact that the entire package is experimental? Or if there's some other way we could apply this globally to all components in the package without having to add it manually to every story?
| component: VisuallyHidden, | |
| component: VisuallyHidden, | |
| tags: [ 'status-experimental' ], |
There was a problem hiding this comment.
I wanted to ask you about this actually. What does the experimental tag mean in the context of @wordpress/ui? Originally, it was to signal that the API could have breaking changes, in normal global packages. We've since moved away from this practice (#59418), preferring "private" when necessary. In @wordpress/ui, we're not going to have a hard back compat policy. My assumption was that there would be no "experimental" status there, at least not in any official way.
There was a problem hiding this comment.
The way I had considered it was as a reflection of the entire package being experimental (pre-1.0), not that we'd be making component-by-component maturity decisions. If we think this is already sufficiently communicated through the package documentation (the banner at the top of the README), I think it'd be okay to remove the indicator in the Storybook.
There was a problem hiding this comment.
I see, that makes sense. I'll remove all the tags, and maybe add an overview doc (re-export of README?) at the root of the Storybook section.
There was a problem hiding this comment.
I see, that makes sense. I'll remove all the tags, and maybe add an overview doc (re-export of README?) at the root of the Storybook section.
Yeah, definitely think we should have some landing pages 👍 I think I have a local branch sitting around somewhere with many of those changes I could push up and we could iterate from.
There was a problem hiding this comment.
I see, that makes sense. I'll remove all the tags, and maybe add an overview doc (re-export of README?) at the root of the Storybook section.
Yeah, definitely think we should have some landing pages 👍 I think I have a local branch sitting around somewhere with many of those changes I could push up and we could iterate from.
I pushed up what I had in case it's useful for reference or iterating from:
https://github.com/WordPress/gutenberg/compare/add/ds-storybook-pages
Prerequisite for #74178
What?
Adds a new
VisuallyHiddencomponent to@wordpress/ui.Why?
This is a replacement for the
VisuallyHiddencomponent in@wordpress/components. It doesn't have an Emotion dependency, and relies on Base UI to support arenderprop that will be standard for the@wordpress/uicomponents.Testing Instructions
See the Storybook story.