Component System: Add FormGroup + ControlLabel components #28568
Conversation
ItsJonQ
added
[Package] Components
|
Size Change: -8 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
@saramarcondes Haiii! I've added the files to |
|
@ItsJonQ Because |
|
@saramarcondes Thanks for the heads up! The comment about Will do my best with the prop JSDoc type definitions |
|
@saramarcondes Haii!! I've added types via JSDocs to the I'm seeing this error:
Any insight would be helpful! Thank you 🙏 |
|
@ItsJonQ You did it just fine! It's just that the JSDoc parser that is used for our JSDoc linter doesn't understand complex types like function signatures and imports. If you check out other parts of the code based that are typed you'll notice that they're littered with cc @sirreal @gziolo have y'all given any thought to disabling that rule? |
Yes, I noticed that for other G2 components as well. It looks like we are reaching the limit for those JSDoc rules. I would personally seek ways to disable them for those files that are typed with TypeScript. As far as I understand those two duplicate their work at the moment. We have |
|
@gziolo That sounds promising, however I'm not familiar with how to disable rules using file patterns in that way. |
|
When we're typechecking a package, the JSDoc type linter to be is totally irrelevant as far as I can tell. TypeScript tooling handles checking the types. For packages that are typed or untyped, this is easy. The lint rule is on or off. The problem is that we have some packages now that are progressively being type, e.g. components. I don't think we want to disable the lint rule for these partially typed packages. A good middle ground would be to disable it for the packages that are completely typed so they don't have to use so many eslint disables. Unfortunately, I think this would have to be a manual list. Doing anything more advanced like trying to parse the |
Can we use the following match as a way to disable those rules? "include": [ "src/**/*" ]It would cover the case when some packages are partially typed. We would have to detect all such config files and use their parent folders to construct the rule for overrides. I can prototype something. |
Looking at Our convention is to write types to the gutenberg/packages/a11y/package.json Line 26 in 92baa9b |
I opened #28729 to address the issue with ESLint and TS types. It doesn't help with this PR just yet, but overall it should be a step in the right direction. |
sarayourfriend
force-pushed
the
add/form-group-component
branch
from
bfdd7a5 to
ef012fd
Compare
|
With VStack integrated (thanks @saramarcondes ), we can continue with this PR 🎉 |
|
Local Storybook testing can found here: After running |
|
Should we see if @diegohaz can do an a11y review for us? I think we've got our bases covered but just to make sure. |
|
There was a problem hiding this comment.
This looks really good overall, but I noticed two issues with the help text:
-
The element should be associated with the input using
aria-describedby="help-text-id"on the input. -
The help text color doesn't meet the minimum contrast ratio required for WCAG 2.1 AA on a white background. You can check this with the Chrome Web Inspector:
I guess
#767676or darker should fix this.
|
@diegohaz Amazing! Thank you for checking 😍 We have a design token for I can make the adjustment there, and it should ripple across the system |
|
Created a PR here: Update: I wonder if it's about time we bring in |
I'm ready for this, but would want to check with @gziolo first to see if we're far enough along for it to be worth it. We have run into several times where we had to make updates and publish new versions of the style system though, so it'd be nice if those just lived in the |
That's fair :)
Yessss 🙌 |
|
Haii! With the recent push, the contrast for the |
|
How are folks feeling with the latest updates :D |
Do we have all components moved that |
|
These are dependencies of other components that are used. Many components in the G2 system rely on context to render one way or another ( |
I find it a bit concerning. I hope that the |
|
I think you're right that we're just hitting on the particularly complex parts of G2 with |
|
I'm not sure if it would work for G2, but one possible solution could be creating more specific components when they depend on some other component's context. For example, |
This update adds a couple of low-level Form components for the new Component System, as part of #28399
This 2 components are at the heart of creating form-based UI, such as those found with attribute controls / design tools:
Compatible components (coming soon) would automatically be bound with labels (as well as
htmlForids for a11y).This is done through the internal
FormGroupContext, resulting in a very minimal component API setup that looks like this:The example above would automatically render a
labeland a uniqueid. Thatidwould then be bound between thelabel(viahtmlFor) and the<TextInput />.In the next Component System, all form based components are pre-connected to this
FormGroupContext, allowing them to work out of the box with zero setup.How has this been tested?
Locally in Storybook + Jest.
This component is already used in the next
FontSizePicker, which has been tested in Gutenberg.Local Storybook testing can found here:
http://localhost:50240/?path=/story/components-formgroup--default
After running
npm run storybook:devTypes of changes
useInstanceIdhook to add a preferredId argumentControlLabelandFormGroupfrom@wp-g2/componentsChecklist: