components: Promote VisuallyHidden from ui into full components #31244
Conversation
sarayourfriend
added
[Package] Components
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
| * | ||
| * function Example() { | ||
| * return ( | ||
| * <VisuallyHidden> | ||
| * <View as="label">Code is Poetry</View> | ||
| * <label>Code is Poetry</label> | ||
| * </VisuallyHidden> | ||
| * ); | ||
| * } |
There was a problem hiding this comment.
I just noticed this createComponent util. Looking at its code, it seems it doesn't do much and basically coping that component here would make things simpler for me at least.
What do you think about it? Is it a good abstraction to have?
There was a problem hiding this comment.
For instance why useVisuallyHidden is a hook in the first place, it just adds a class, so why not just add it inline?
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 Context or adding extra components to the React tree for example).
Additionally it's used for a ton of components in ui and refactoring them all to just copy the code from createComponent would increase the maintenance cost of all those components.
There was a problem hiding this comment.
makes us think twice before using Context or adding extra components to the React tree for example
Why is this a bad thing?
There was a problem hiding this comment.
Additionally, it enforces the use of View where we will be able to add back the base styles that we lost by moving away from the custom style system. Because everything is ultimately a View we can just apply the base styles there.
There was a problem hiding this comment.
Now looking more at createCompnent, It does call memo by default in all components, that personally doesn't make sense to me. It's going to do the opposite of what it's meant to solve. Basically, most of these small components don't need to be memoized, React already only update the underlying Dom element if its prop changes, so this extra memo is useless. At least we should change the default value to false there.
There was a problem hiding this comment.
My main worry is that it's an abstraction that doesn't bring much while raising the bar for casual components to understand how components are built in this package, why it's just just a regular component.
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 Text component looking if we removed createComponent? Would we get rid of the useText hook?
My concern is that if we remove createComponent, we'll just be duplicating a ton of code everywhere, indeed, that's why Reakit uses it for example. If a component is follows the pattern established in createComponent, namely:
- Push all props through a hook
- Pass all processed props to View
- Connect to the contextSystem
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.
How do you envision, for example, the Text component looking if we removed createComponent? Would we get rid of the useText hook?
Well! for me these useVisuallyHidden, useText... hooks are very weird hooks. They don't really match what hooks are meant to be used for. They correspond to "components" and not "behavior". It looks like a hack to the hooks system :P
Push all props through a hook
What is this exactly? :P Is it a requirement to pass all props through a hook?
Push 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 Icon component support that? an Icon always renders an svg (just an example)
Connect to the contextSystem
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.
At least we should change the default value to false there.
Agreed! We need to update that in contextConnect as well.
There was a problem hiding this comment.
It looks like a hack to the hooks system :P
That's fair, they probably are 😅
Is it a requirement to pass all props through a hook?
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 useText without rendering a Text element (like useHeading for example, does everything a Text can do but with some extras, same with VStack, it's just HStack with some extra props, which in turn is just Flex configured a specific way), this is now possible using these "hacky" hooks.
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).
Do all components need to support "as" and other features proposed by the "View" component
I think so. It's hard to decide what components don't benefit from the as prop. It's an extremely powerful tool for composition.
Icon itself probably doesn't need it, you're correct, but I think the exceptions are few.
Do all components need to connect to the context system
You'd be surprised. We even have examples of Text being accessed via the context system.
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
| /** | ||
| * @param {import('../context').ViewOwnProps<{}, 'div'>} props | ||
| * @param {import('../ui/context').ViewOwnProps<{}, 'div'>} props | ||
| */ | ||
| export function useVisuallyHidden( { className, ...props } ) { | ||
| // circumvent the context system and write the classnames ourselves |
There was a problem hiding this comment.
should we remove the wp-components-visually-hidden classname?
| @@ -1,30 +0,0 @@ | |||
| .components-visually-hidden { | |||
There was a problem hiding this comment.
Removing all these styles could potentially impact some third-party plugins that rely on them. The good news is that these are not official APIs, meaning plugins should be using components and not classnames. I think once we make more progress into merging G2 components and removing these old styles, we should consider writing a dev note to document the removed classnames.
|
|
||
| export default withNextComponent( forwardRef( VisuallyHidden ) ); | ||
| export { default as VisuallyHidden } from './component'; | ||
| 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.
| @@ -18,7 +18,7 @@ function FormGroupLabel( { children, id, labelHidden = false, ...props } ) { | |||
|
|
|||
| if ( labelHidden ) { | |||
| return ( | |||
| <VisuallyHidden as="label" htmlFor={ id }> | |||
| <VisuallyHidden as="label" htmlFor={ id?.toString() }> | |||
There was a problem hiding this comment.
Curious why this change was needed?
There was a problem hiding this comment.
The previous prop types were not comprehensive so they didn't have strict checking of htmlFor. But id comes in as either a number | string and htmlFor, when properly type checked against a label only 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 PolymorphicComponent that has ViewOwnProps for its arguments, it actually gets the correct full prop types when using the as prop.
There was a problem hiding this comment.
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.
Yup! It'll still work, just a type issue.
sarayourfriend
force-pushed
the
add/upgrade-visually-hidden
branch
from
61d8ee1 to
a529105
Compare
|
Size Change: -445 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
sarayourfriend
force-pushed
the
add/upgrade-visually-hidden
branch
from
a529105 to
dfdbe11
Compare
sarayourfriend
force-pushed
the
add/upgrade-visually-hidden
branch
from
dfdbe11 to
9c6aa5e
Compare
…components (#31244)"" (#31902) * Revert "Revert "components: Promote VisuallyHidden from ui into full components (#31244)" (#31882)" This reverts commit 269f31a. * Convert to styled component * Update snapshots * Add children prop * Use View naming convention * Use inline styles and remove focus styles * Update color picker snapshots * Render a View to allow for polymorphism and forward the ref
Description
Promotes
ui/visually-hiddenintocomponents/src/visually-hiddenand exports it as the canonical VisuallyHidden for wp/components.Please test this PR, test out a few places that use VisuallyHidden 🙂 They have the exact same API so there should be no differences between the two but just in case!
How has this been tested?
Storybook, unit tests
Types of changes
New feature! This actually expands the API of VisuallyHidden to have a properly implemented
asand polymorphism. Not sure if we should include that in the CHANGELOG.Checklist:
*.native.jsfiles for terms that need renaming or removal).