Components: Allow extra props for RadioControl component #28631
Conversation
Add Extra Props for disabled attribute like
ser-manjeet
requested review from
ajitbohra and
chrisvanpatten
as code owners
github-actions
bot
added
the
First-time Contributor
gziolo
added
the
[Status] Needs More Info
|
What props are exactly missing? Should they be part of the public API maybe? @ItsJonQ, how optional props are handled in G2 components? Ideally, it should be either all components take custom props or only explicitly defined props are allowed. |
There was a problem hiding this comment.
This LGTM, thanks for the contribution!
@gziolo I think there are basically two classes of components in WordPress components. @ItsJonQ and I have discussed this before a couple of times. There are "primitive" components, like Button and the various Controls and there are UI components (I have no idea what to call these, I've seen them called UI Blocks before but "blocks" is too overloaded in the Gutenberg context). Each of these have different considerations as far as prop forwarding goes. For the primitive components, I think we should forward props as this PR does because it allows the consumer to fully control the underlying component without requiring us to document each and every single DOM attribute. G2 does this via the context system (and we've even successfully type annotated this through the PolymorphicComponent type). On the other hand, UI components like Placeholder, AnglePickerControl, or FontSizeControl don't make sense to do this for because... well, where would we forward those props onto? I think in the case of those components, they're so specific that they should just be used as is, rather than the more general "primitive" components.
Hopefully that makes sense. This is definitely just something that @ItsJonQ and I came up with to explain our thought process as this was discussed during the FontSizeControl intregration (we narrowed the props in that case) so it's something work having an ongoing discussion about, especially around the language we use to denote each type of component we have in this library.
What props are exactly missing? Should they be part of the public API maybe?
In thise case all DOM attribute props are missing (disabled, aria props, any additional data attributes the consumer may want to apply, etc). To document this as part of the API we merely have to add & React.HTMLProps<'div'> to the final prop type for this component.
gziolo
removed
the
[Status] Needs More Info
gziolo
changed the title
gziolo
changed the title
|
Size Change: +2.87 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
@saramarcondes – thank you for your detailed response. I agree with the reasoning. It all makes sense in the context of 3rd party usage. For those low-level/primitive components, it all makes sense to allow any props to be passed as an advanced use case.
All of the reasons to add props you mentioned are very much specific to the web. Do we plan to make G2 components cross-platform? At the moment it looks like they are for usage with the web only. If the answer is yes, we should consider a translation layer for stuff that becomes problematic like styles or accessibility-related attributes. It probably should be its own discussion though 😄 |
|
Congratulations on your first merged pull request, @ser-manjeet! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
Shall we start an issue to discuss this? I don't know what the plan is for this at all. cc @ItsJonQ |
|
It all depends on the mobile team if they plan to invest time in making G2 components cross-platform. Regardless, it probably is s good idea to open an issue to have this discussion well documented. |
@gziolo As @saramarcondes had mentioned, I'm not sure what the plan is for this. However, I do feel strongly that Component library UI should provide the ability to render attributes accepted by HTML (e.g. Examples may include: Adding fragile styles in the tune of As mentioned, this does mean that web props may not translate well into React Native.
During this process, props are filtered out: This implementation is based on Emotion, and is a standard process for CSS-in-JS solutions. Interesting, it appears that other libraries have standardized around using For RN, we could adjust this to accommodate RN components - either remapping certain properties or filtering them out entirely.
Sounds good :) |
|
@ser-manjeet I've tried to reach out via other channels, but I have been unsuccessful in getting in touch. I wanted to make sure you saw the request to you for consent to re-license your contributions to Gutenberg under GPLv2 and MPLv2 here: #31893. If you would kindly review that description and comment accordingly it would be greatly appreciated. Thanks! |
Description
This PR allows extra props for RadioControl component like
disabledor ARIA related props.How has this been tested?
Screenshots
Types of changes
It's an enhancement that aligns the component's API with other components.
Checklist: