ENYO-5153: Fix Button's sampler to toggle minWidth correctly

[viodragon2]

Issue Resolved / Feature Added

minWidth prop's default value is set to true, and by nullifying the boolean value, it will always set to true even if the knob is set to false. Fixed by removing nullifying in the select knob.

Additional Considerations

• Button's props weren't being properly merged in the info section and made an update to it. It seems like other components that uses ui as a base component show similar pattern and we need to update them all.
• Any props that were added by HOCs (e.g. casing prop from i18n/Uppercase) will not be fetched and need to be set with constants. Should we declare those propTypes in the base component or is there a way to fetch them and feed it to propTables in the story?

Enact-DCO-1.0-Signed-off-by: Stephen Choi stephen.choi@lge.com

[Djspaceg]

Having no blank ('') option implies that the component must use this prop. backgroundOpacity is optional and the sample should represent that. I'm fine with also having a blank/opaque option, but not ok with replacing the blank with 'opaque'.

[viodragon2]

Similar to the comment below, it seemed awkward to me when Button's backgroundOpacity has @default 'opaque in the documentation, but not set anything in the sampler. Plus, it is documented as one of the options but the user cannot select it in the sampler. I thought that was inconsistent. But if we remove from one of the options in propTypes as you mentioned, should '' be one of the options instead?

[Djspaceg]

I agree. Let's remove the @default 'opaque' from the docs. It was put there as a way to explain that the "normal" state of a Button's appearance is opaque, and not translucent, so PropTypes wouldn't error if someone attempted to use a logical word for an opacity property, even though it's just, and already, the default.

'' in this case, due to nullify, below removes backgroundOpacity from the JSX entirely, so it appears as <Button> rather than implying there's a required property: <Button backgroundOpacity="opaque">.

We're getting into "developer experience" (DX) territory here, rather than discussing minWidth. A fundamental question like "should an empty prop value be enumerated in an array of valid options so it's clear to a developer that they can specify nothing and get the default on a non-required property". My preference is "no", because as a developer I want to see what I'm practically able to set a prop to, not an exhaustive set of values. (We don't list null or undefined as options, even though those technically also work.) If I choose to not set it in my app, I'll just exclude that prop from my code and expect it to work. I definitely don't want developers to think they must assign an empty value: <Button backgroundOpacity=""> to make a simple button work. That being said, we do have to sort of play nicely with Storybook stories, and massage them into accurately representing our options. There is no way to temporarily remove a knob from the Knobs list, so we need a way to indicate, in our existing select knob, that not providing a prop is a valid option. This is where the '' comes in (as the best, most representative option we've found so far). It's necessary to show that not choosing a value is an acceptable option.

"opaque" was not listed as a knob value because I didn't want to cause confusion about what the values mean. If '' and "opaque" are both present, it could imply that they should visually represent two different states, i.e. whatever the default/blank is, and opaque, which must be different, because it's listed as a separate option.

If we must change the documentation, PropTypes, and Story, I'd personally prefer to remove the @default 'opaque' and leave the '' in the knob values. The documentation will also need to be updated to describe that Button, by default, is visually opaque but that an "opaque" string value is not a valid option for the backgroundOpacity prop, so it's clear what is going on.

[Djspaceg]

I'd almost prefer that opaque be removed from the available options, rather than have it be always applied as a default, so the default (and default applied class) can just the normal resting state of the Button. "Opaque" was originally added just for consistency if we needed it, but we don't need it yet at this point.

[viodragon2]

From documentation perspective, it is set to @default 'opaque'. But it seemed the property wasn't being set when rendered. Wanted to fill in the gap. I think it'd be okay to remove the defaultProps as long as it is aligned with the documentation.

[Djspaceg]

The literal null can (should) be omitted entirely from this list, since a null is implied by simply not including the backgroundOpacity property. We're not saying that <Button backgroundOpacity={null}> should be used in practice, but instead saying that <Button> is a valid option. If this prop were "required", and it needed to support a non-value (for whatever reason), then I'd say we should to keep the null literal.

[viodragon2]

I had a bit of confusion with color having null value as an option. I'll go ahead and make that change as well.

FYI: I don't think there's a good way to include null as prop type when isRequired. See facebook/react#3163

[Djspaceg]

Looks good! Thanks.

[Djspaceg]

👍 Good catch. Didn't see this one initially.