components: Promote VisuallyHidden from ui into full components #31244
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
|
||
| exports[`Basic rendering should render 1`] = `"<div tabindex=\\"-1\\" class=\\"block-editor-link-control\\"><div class=\\"block-editor-link-control__search-input-wrapper\\"><form><div class=\\"components-base-control block-editor-url-input block-editor-link-control__search-input css-wdf2ti-Wrapper e1puf3u0\\"><div class=\\"components-base-control__field css-11vcxb9-StyledField e1puf3u1\\"><input required=\\"\\" class=\\"block-editor-url-input__input\\" type=\\"text\\" placeholder=\\"Search or type url\\" role=\\"combobox\\" aria-label=\\"URL\\" aria-expanded=\\"false\\" aria-autocomplete=\\"list\\" aria-owns=\\"block-editor-url-input-suggestions-0\\" value=\\"\\"></div></div><div class=\\"block-editor-link-control__search-actions\\"><button type=\\"submit\\" class=\\"components-button block-editor-link-control__search-submit has-icon\\" aria-label=\\"Submit\\"><svg width=\\"24\\" height=\\"24\\" xmlns=\\"http://www.w3.org/2000/svg\\" viewBox=\\"-2 -2 24 24\\" role=\\"img\\" aria-hidden=\\"true\\" focusable=\\"false\\"><path d=\\"M6.734 16.106l2.176-2.38-1.093-1.028-3.846 4.158 3.846 4.157 1.093-1.027-2.176-2.38h2.811c1.125 0 2.25.03 3.374 0 1.428-.001 3.362-.25 4.963-1.277 1.66-1.065 2.868-2.906 2.868-5.859 0-2.479-1.327-4.896-3.65-5.93-1.82-.813-3.044-.8-4.806-.788l-.567.002v1.5c.184 0 .368 0 .553-.002 1.82-.007 2.704-.014 4.21.657 1.854.827 2.76 2.657 2.76 4.561 0 2.472-.973 3.824-2.178 4.596-1.258.807-2.864 1.04-4.163 1.04h-.02c-1.115.03-2.229 0-3.344 0H6.734z\\"></path></svg></button></div></form></div><fieldset class=\\"block-editor-link-control__settings\\"><legend class=\\"components-visually-hidden css-1r162ic-VisuallyHidden css-1mm2cvy-View egi4jkx0\\">Currently selected link settings</legend><div class=\\"components-base-control components-toggle-control block-editor-link-control__setting css-wdf2ti-Wrapper e1puf3u0\\"><div class=\\"components-base-control__field css-11vcxb9-StyledField e1puf3u1\\"><span class=\\"components-form-toggle\\"><input class=\\"components-form-toggle__input\\" id=\\"inspector-toggle-control-0\\" type=\\"checkbox\\"><span class=\\"components-form-toggle__track\\"></span><span class=\\"components-form-toggle__thumb\\"></span></span><label for=\\"inspector-toggle-control-0\\" class=\\"components-toggle-control__label\\">Open in new tab</label></div></div></fieldset></div>"`; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,4 +46,3 @@ | |
| @import "./toolbar-button/style.scss"; | ||
| @import "./toolbar-group/style.scss"; | ||
| @import "./tooltip/style.scss"; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,4 +14,3 @@ export * from './text'; | |
| export * from './truncate'; | ||
| export * from './v-stack'; | ||
| export * from './view'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,17 @@ | ||
| # VisuallyHidden | ||
|
|
||
| `VisuallyHidden` is a component used to render text intended to be visually hidden, but will show for alternate devices, for example a screen reader. | ||
|
|
||
| ## Usage | ||
|
|
||
| ```jsx | ||
| import { VisuallyHidden } from '@wordpress/components'; | ||
|
|
||
| function Example() { | ||
| return ( | ||
| <VisuallyHidden> | ||
| <label>Code is Poetry</label> | ||
| </VisuallyHidden> | ||
| ); | ||
| } | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { createComponent } from '../ui/utils'; | ||
| import { useVisuallyHidden } from './hook'; | ||
|
|
||
| /** | ||
|
|
@@ -10,12 +10,12 @@ import { useVisuallyHidden } from './hook'; | |
| * | ||
| * @example | ||
| * ```jsx | ||
| * import { VisuallyHidden } from `@wordpress/components`; | ||
| * | ||
| * function Example() { | ||
| * return ( | ||
| * <VisuallyHidden> | ||
| * <label>Code is Poetry</label> | ||
| * </VisuallyHidden> | ||
| * ); | ||
| * } | ||
|
There was a problem hiding this comment. I just noticed this What do you think about it? Is it a good abstraction to have? There was a problem hiding this comment. For instance why There was a problem hiding this comment. I think it's a good abstraction to have and encourages "pure hook" components, which I think is generally a good thing (makes us think twice before using Additionally it's used for a ton of components in There was a problem hiding this comment.
Why is this a bad thing? There was a problem hiding this comment. Additionally, it enforces the use of There was a problem hiding this comment. Now looking more at createCompnent, It does call There was a problem hiding this comment.
That's fair, it is a new abstraction and does technically redirect from the core functionality of a component. How do you envision, for example, the My concern is that if we remove
Then we've just duplicated that code, well, more than duplicated, we've replicated probably dozens of times over across the components codebase, just to avoid a tidy, well-tested abstraction. Is that worth it? 🤷♀️ There was a problem hiding this comment.
Well! for me these
What is this exactly? :P Is it a requirement to pass all props through a hook?
Do all components need to support "as" and other features proposed by the "View" component? I guess my opinion is probably "no" Why should an
Do all components need to connect to the context system. Isn't that just useless overhead for most components? Not sure I understand that system well enough but it seems only useful for components that compose (Card, CardBody...) There was a problem hiding this comment.
Agreed! We need to update that in There was a problem hiding this comment.
That's fair, they probably are 😅
No, but it's just the way that the g2 components were designed from a technical perspective, to encapsulate all, or as much as possible, logic into a hook that can be reused by other components. For example, if another component wanted to implement the same logic as I'm not saying it's not complicated, but I think approach the technical design of components in this way does add some value around composibility within the library itself (without necessarily exposing all the details of that to consumers of the library).
I think so. It's hard to decide what components don't benefit from the Icon itself probably doesn't need it, you're correct, but I think the exceptions are few.
You'd be surprised. We even have examples of I have wondered about the increased overhead of the context system but I think ultimately it's negligible. There were other things like memoization that apparently yielded more significant performance benefits: ItsJonQ/g2#57 |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,46 +1,2 @@ | ||
| export { default } from './component'; | ||
|
There was a problem hiding this comment. Why do we export the component twice? There was a problem hiding this comment. Some places were importing it as the default and others were importing it by name, so I figured this would minimize the number of changes elsewhere in the components package. I can switch to just one or the other if you prefer though (probably just the named export is best?) There was a problem hiding this comment. I think we historically started with default exports for components. Even if I personally would prefer named components now, I think it's probably better to stay consistent and use default export here. |
||
| export * from './hook'; | ||
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.
Curious why this change was needed?
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.
The previous prop types were not comprehensive so they didn't have strict checking of htmlFor. But
idcomes in as either anumber | stringandhtmlFor, when properly type checked against alabelonly accepts strings.The polymorphism that existed in the previous component implementation didn't actually make any difference to the types, but now that it's a true
PolymorphicComponentthat hasViewOwnPropsfor its arguments, it actually gets the correct full prop types when using theasprop.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.
Question though, it's just for typing right? I mean, a user passing a numeric value there will still work? (thinking about BC)
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.
Yup! It'll still work, just a type issue.