Skip to content

Commit

Permalink
Add condition for checked and unchecked labels
Browse files Browse the repository at this point in the history 
Components using the FormToggle component can use this to supply help text based on the state of the checkbox.
  • Loading branch information
Joen Asmussen committed Mar 23, 2018
1 parent 14c54cb commit 35fdd72
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 2 deletions.
2 changes: 2 additions & 0 deletions blocks/library/gallery/block.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ class GalleryBlock extends Component {
label={ __( 'Crop Images' ) }
checked={ !! imageCrop }
onChange={ this.toggleImageCrop }
help={ __( 'Thumbnails are not cropped.' ) }
checkedHelp={ __( 'Thumbnails are cropped to align.' ) }
/>
<SelectControl
label={ __( 'Link to' ) }
Expand Down
10 changes: 8 additions & 2 deletions components/toggle-control/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,15 @@ class ToggleControl extends Component {
}

render() {
const { label, checked, help, instanceId } = this.props;
const { label, checked, help, checkedHelp, instanceId } = this.props;
const id = `inspector-toggle-control-${ instanceId }`;

// allow customizing the help text based on checked state
let helpLabel = help;
if ( checkedHelp ) {
helpLabel = checked ? checkedHelp : help;
}

This comment has been minimized.

Sign in to view

Copy link
@youknowriad

youknowriad Mar 23, 2018

Contributor

It's a bit confusing that the help prop corresponds to the unchecked state. What if we just switch the value from outside?

help={ !! imageCrop ? 'x is checked' : 'x is not checked' }

This comment has been minimized.

Sign in to view

Copy link
@jasmussen

jasmussen Mar 24, 2018

Contributor

While your code is definitely better, the idea here is to make this an intrinsic feature of the toggle-control. I'm aware that anyone can write a ternary operator like this, and accomplish the same, but it's way harder to do so than just filling out another property on a control, and have the component itself handle the logic.

The hope is that we can apply this ultra-contextual helptext in all the block settings where it is helpful. Then plugin developers can look at these block settings for inspiration, and hopefully re-use this pattern.

What do you think? Is there a way we can write the logic into the component itself to make the pattern easier to use?

This comment has been minimized.

Sign in to view

Copy link
@youknowriad

youknowriad Mar 24, 2018 •
edited
Loading

Contributor

I'm aware that anyone can write a ternary operator like this, and accomplish the same, but it's way harder to do so than just filling out another property on a control, and have the component itself handle the logic.

Honestly, I don't think it's harder, I think it's better than using two props that don't convey the correct meaning: help being the prop corresponding to the unchecked state and checkedHelp the prop of the checked state. An alternative would be to support help prop as a function receiving the checked value as a parameter and return the corresponding label. But for me, both of these patterns would have made more sense if the component was uncontrolled, which means the value (checked prop here) is not defined by the outer component but updated in the local state of the ToggleControl component because in uncontrolled component the value changing the help text is not necessarily known outside of the component itself.

This comment has been minimized.

Sign in to view

Copy link
@jasmussen

jasmussen Mar 24, 2018

Contributor

help and checkedHelp are definitely horrendous labels, for absolutely sure. That's just one of the reasons this needs thoughts like those you're giving now. And there are no doubt countless reasons why my code is not great react code... and yes, if in the end we can't find a good solution then I'm fine with reverting this.

So let me instead pose it as a question: how can we make it as easy as possible for developers using the ToggleControl, to pass multiple help strings based on the state of the toggle?

Your code example solves it, but can it be made even smoother, so it feels a part of the component, and not a custom function passed to the component? Is it just a matter of using the pattern ourselves and then writing a good README?

This comment has been minimized.

Sign in to view

Copy link
@youknowriad

youknowriad Mar 24, 2018 •
edited
Loading

Contributor

I see what you're heading with this but for me instead of making it feel as part of the component we should instead try to educate them about how to use React, the React way 😄. For me adding a logic to have help strings based on the state of the toggle would be best supported using a function prop:

help={ ( value ) => value ? 'x is checked' : 'x is not checked' }

But this creates other issues:

  • The value param of the function is a duplicate information because the component is controlled which means, the person using the components is always providing its value.

  • Using function props can cause rerenders (performance loss even if very minor here) because on each render we're passing a new function and React can't know that it's the same prop (unless we bind the function but this is harder to explain to developpers new to React).

That's why I still think the ternary is still the best option.

--
Pinging @aduth @gziolo to have other opinions

This comment has been minimized.

Sign in to view

Copy link
@gziolo

gziolo Mar 26, 2018

Member

I like that the fact that this check happens inside because it simplifies things when you want to use it.
You can simplify implementation to:

const helpLabel = checkedHelp && checked ? checkedHelp : help;

In the majority of cases, you will only provide help so what @youknowriad initially shared makes sense, too. When imageCrop changes then help will also change which is exactly what we want to have when updating props.

I'm fine with both approaches.

This comment has been minimized.

Sign in to view

Copy link
@youknowriad

youknowriad Mar 26, 2018

Contributor

Just to clarify I'm also fine with the proposed approach, I just don't like the prop names and if want it to scale to all controls, we need to do is as a function prop because in a SelectControl we have more than two values (same for text etc...)

This comment has been minimized.

Sign in to view

Copy link
@gziolo

gziolo Mar 26, 2018

Member

Mybe we should do the following:

const helpLabel = isFunction( help ) ? help( checked ) : help;

That would make it both flexible and easy to use in the common flow. I think having a different help message is an edge case.

This comment has been minimized.

Sign in to view

Copy link
@jasmussen

jasmussen Mar 26, 2018

Contributor

Thanks for looking, friends.

The key goal is to make it easy for developers to add a customized help label for the toggle control. How best to do it codewise, I completely defer to you.

If that is a ternary operation, I'd just request we add an example to the README for the component. If it's something else, then I'm totally open to how the function looks and is named.

I'm AFK today with a sick child, so feel free to push to this branch if you like.

This comment has been minimized.

Sign in to view

Copy link
@gziolo

gziolo Mar 26, 2018

Member

I might look into it tomorrow morning, busy with other PRs. Hope your child gets better soon 👍

let describedBy;
if ( help ) {
describedBy = id + '__help';
Expand All @@ -37,7 +43,7 @@ class ToggleControl extends Component {
<BaseControl
label={ label }
id={ id }
help={ help }
help={ helpLabel }
className="components-toggle-control"
>
<FormToggle
Expand Down

0 comments on commit 35fdd72

Please sign in to comment.