Components: Introduce a isDisabled prop to the Disabled component #26730
Conversation
tyxla
added
[Package] Components
|
Size Change: +2.99 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
Could it not be a prop of the existing |
|
Thanks for the review, @ellatrix 🙌
It totally could be! The main rationale for introducing a new component was naming semantics. For example, I find this way more intuitive: than: because we avoid the "disabled" tautology, plus it looks a bit counter-intuitive to have a In any case, if you'd recommend moving the
That's true. The main reason to prefer wrapping in a |
|
I don't know, to me a |
I don't understand, returning it twice? The fragment seems to add unnecessary nesting to the React tree. |
tyxla
force-pushed
the
add/components-disableable
branch
from
2daf41f to
ed70ce1
Compare
tyxla
changed the title
|
Would be great if I could get a review here from anyone who has a spare cycle. @adamziel @youknowriad any idea who's best to ping for that? |
|
I don't mind having such prop - it makes life easier even if it is a tiny bit awkward. This here is very similar to a discussion we've once had about |
|
@tyxla I understand you wanted to avoid the following code? const DisabledMaybe = shouldBeDisabled ? Disabled : Fragment;
return (
<DisabledMaybe>
// ...
</DisabledMaybe>
) |
| @@ -133,6 +133,46 @@ describe( 'Disabled', () => { | |||
| expect( div.hasAttribute( 'tabindex' ) ).toBe( true ); | |||
| } ); | |||
|
|
|||
| it( 'will disable descendant fields if isDisabled prop is set to true', () => { | |||
There was a problem hiding this comment.
I think we can try to merge these two test cases together and simulate a state update from isDisabled={true} to isDisabled={false} in between. Just to make sure that the descendant fully updated.
There was a problem hiding this comment.
Isn't this already covered in the previous test?
There was a problem hiding this comment.
That test tested from wrapping <Disabled> to not wrapping it. What I expect here is to test it from isDisabled={true} to isDisabled={false} (or the other way around) while always wrapping it with <Disabled>. Makes sense :)?
There was a problem hiding this comment.
Thanks for elaborating @kevin940726 - that makes sense! I'll update the tests shortly.
There was a problem hiding this comment.
Updated in 11c1a62. Thanks again!
tyxla
force-pushed
the
add/components-disableable
branch
from
ed70ce1 to
4461198
Compare
Yes, exactly. While it's extremely simple, it could be very useful to avoid writing it every time one uses |
|
I think all the feedback has been covered here, could I ask for another review when you folks get a chance? |
tyxla
requested review from
kevin940726 and
adamziel
and removed request for
adamziel and
kevin940726
youknowriad
added
[Type] New API
Description
A common pattern in projects is to receive the
disabledform state from component state or passed props. Then, in order to make one or more@wordpress/componentscomponents disabled, we conditionally wrap them in a<Disabled />component, or in a<Fragment />. And any time we want to do that, we have to wrap the<Disabled />element with such a condition. This can actually be seen in the existing example in the documentation of<Disabled />.To make this more intuitive, this PR introduces a
isDisabledprop to the<Disabled />component. Based on its boolean value (which defaults totrue), the<Disabled />component will now decide whether to disable all itschildrenor not.How has this been tested?
npm run storybook:devhttp://localhost:50240/?path=/story/components-disabled--disableable(true)|(false)" button, verify both "disabled" and "non-disabled" states work as expected and disable/enable nested fields.npm run test-unit /packages/components/src/disabledScreenshots
This PR doesn't introduce any visual changes.
Types of changes
This PR introduces a new
isDisabledprop to the<Disabled />component.Checklist: