Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create v6 HelpBox with Styled System #414

Merged
merged 8 commits into from
Dec 31, 2020

Conversation

TyMick
Copy link
Collaborator

@TyMick TyMick commented Dec 15, 2020

Onward #378!

HelpBox had previously been using a legacy theme prop and a homespun variant API, so I replaced those with our Styled System global theme and Styled System's variant API.

I created a variant prop for these variants but also kept the primary, success, danger, warning, and minor props as shortcuts, and I added a perhaps somewhat convoluted function called validateVariantSelection to handle conflicts between those props if they're used incorrectly. Let me know if I should remove those shortcut props.

Also, should I enable any regular Styled System categories for this component? None are enabled currently.

@TyMick
Copy link
Collaborator Author

TyMick commented Dec 16, 2020

Oops, I just realized that there's a getVariation function that already exists, so I've swapped my validateVariantSelection out for that one. I also added some documentation to getVariation while I was at it and changed the name of its second parameter—does this look alright?

/**
 * Chooses the correct variant from a main variant prop and a set of boolean shortcut props.
 *
 * @param {string|undefined} variant - The value of the component's `variant` prop.
 * @param {{ [propName: string]: boolean }} shortcutProps - An object of shortcut prop names and boolean values, the names corresponding to variant name options.
 * @returns {string|null} The name of the selected variant (or `null` if no variant has been selected).
 */

The one thing my validateVariantSelection had that getVariation doesn't was console warnings in development whenever variant props conflict (if multiple shortcut props are used, if variant is set to one value and a shortcut prop of a different variant is also used, etc.). Would that be a valuable addition to getVariation or should I not bother?

@TyMick
Copy link
Collaborator Author

TyMick commented Dec 18, 2020

I didn't add anything about the new variant prop to the upgrade guide, since it's currently a non-breaking change (shortcut props are still enabled). Let me know if we want non-breaking changes in the upgrade guide as well, though, or if we want to deprecate or remove the shortcut props.

@RobertBolender RobertBolender mentioned this pull request Dec 22, 2020
40 tasks
@RobertBolender
Copy link
Contributor

I created a variant prop for these variants

Beautiful 👍

Oops, I just realized that there's a getVariation function that already exists

Glad we can keep using that one 👍

I also added some documentation to getVariation while I was at it and changed the name of its second parameter—does this look alright?

Looks beautiful 👍

The one thing my validateVariantSelection had that getVariation doesn't was console warnings in development whenever variant props conflict

I'm not overly concerned about that scenario.

Let me know if we want non-breaking changes in the upgrade guide as well, though, or if we want to deprecate or remove the shortcut props.

I was about to say let's remove them... but I don't feel strongly enough to do so. I'm fine to keep them. We can always deprecate them for v7 if we feel the need.

@RobertBolender RobertBolender merged commit ed9ec91 into Faithlife:master Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants