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

feat(helper-text): added helper text component #4089

Merged
merged 6 commits into from
May 26, 2021

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented May 20, 2021

fixes #4017

Preview - https://patternfly-pr-4089.surge.sh/components/helper-text

Adds 2 types of helper texts:

Static

This is the default helper text. With static helper text, the text color and icon color are the same. By default the text and optional icon are black. Indeterminate is grey, warning is yellow, success is green, invalid is red.

Dynamic

This is an opt in to the helper text component. By default a dynamic helper text and icon are black. Intermediate is grey, warning has yellow icon and grey text, success has green icon and grey text, and invalid has red icon and black text.

A helper text component is a block made up of helper text items. There can be one or more items, and if there are multiple items in a helper text block, there is a xs spacer between items. A block of multiple items can be regular divs (in the case of regular helper text that isn't necessarily a list), or an HTML list (particularly useful in dynamic/validation helper text).

@mcoker mcoker requested review from mcarrano and doruskova May 20, 2021 23:03
@patternfly-build
Copy link

patternfly-build commented May 20, 2021

Preview: https://patternfly-pr-4089.surge.sh

A11y report: https://patternfly-pr-4089-coverage.surge.sh

CSS Size Report
NameCurrentPreviousDiff %
components/HelperText/helper-text.css4.5 kBNaN kBNaN
patternfly-no-reset.css1.2 MB1.2 MB0.39
patternfly.css1.2 MB1.2 MB0.39
patternfly.min.css1.1 MB1.0 MB0.40

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good @mcoker . My only feedback is that I think the yellow/warning version should use the darker gold text color for better contrast/readability. @mceledonia @doruskova @ajacobs21e can you also take a look?

Copy link

@doruskova doruskova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @mcarrano. The warning text should use --pf-global--warning-color--200 because the light yellow is not accessible. However, it looks great! Thank you!

@mcoker
Copy link
Contributor Author

mcoker commented May 24, 2021

@mcarrano @doruskova thanks! Looking at the alert, we use success/warning/danger-color--100 for the icon and success/warning/danger-color--200 for the text. Should I use that here?

In the form component, the error helper text and icon both are danger-color--100, however I'm not sure that's the correct implementation. We do not have a unique icon color for the form component helper text for any of the states. This is what it looks like currently - the success color is success-color--200, warning is warning-color--200.

Screen Shot 2021-05-24 at 11 05 24 AM

Here is this helper text PR with icon/text color that matches the alert component with success/warning/danger-color--100 for the icon and success/warning/danger-color--200 for the text.

Screen Shot 2021-05-24 at 11 06 08 AM

@mcarrano
Copy link
Member

Yep. I think this is the way to go @mcoker .

@mcoker
Copy link
Contributor Author

mcoker commented May 24, 2021

@mcarrano great, those color updates are in the PR now.

@mcoker mcoker requested a review from mattnolting May 24, 2021 17:37
@mcoker
Copy link
Contributor Author

mcoker commented May 24, 2021

@mcarrano btw I called this component "Helper text" - is that OK or do you want to name it "Rich helper text"?

@mcarrano
Copy link
Member

I think that the name is fine @mcoker . It's more likely people will look for it under this name.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@doruskova
Copy link

@mcoker Looks great!

Copy link
Contributor

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mcoker, this is looking great! Couple of questions

  • Not sure I understand the static/dynamic concept. It looks like they could be used interchangeably, based on messaging and/or intent.

In this example, I would assume both helper texts are dynamic, but I wouldn't inherently know which variant to use.

Screen Shot 2021-05-25 at 9 18 45 AM

I think this could be solved by either changing dynamic to another name or adding documentation explaining when to use which variant.

  • Looks like you need to add standard documentation at the end of HelperText.md

Comment on lines +70 to +73
&.pf-m-dynamic {
--pf-c-helper-text__item-text--Color: var(--pf-c-helper-text--m-dynamic__item-text--Color);
--pf-c-helper-text__item-icon--Color: var(--pf-c-helper-text--m-dynamic__item-icon--Color);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

@mcoker
Copy link
Contributor Author

mcoker commented May 25, 2021

@mcarrano wdyt about @mattnolting's comment about naming above? I would expect the design docs to explain the usage, but we can also explain the usage a bit in the component documentation at the bottom of the component, and rename the variation if it could be more clear as to what the variation does. The react implementation will also illustrate the difference better as the dynamic variation changes as the user types in a field.

@mcarrano
Copy link
Member

@mcoker adding a bit of explanation might help. Seems like the distinction is more of a behavioral one than anything to do with the structure of the component, right? So I can see why this distinction might seem unnecessary to someone just using the HTML.

@mcoker
Copy link
Contributor Author

mcoker commented May 25, 2021

@mattnolting updated the docs

@mcoker
Copy link
Contributor Author

mcoker commented May 26, 2021

@mattnolting I chatted with @mcarrano offline about the confusion using the dynamic modifier, and didn't have an obvious solution. I'm fine with merging it as is and following up if we need to. wdyt?

Copy link
Contributor

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LAPTM 😄 Let's follow up w/naming the variant.

@mattnolting
Copy link
Contributor

@mattnolting I chatted with @mcarrano offline about the confusion using the dynamic modifier, and didn't have an obvious solution. I'm fine with merging it as is and following up if we need to. wdyt?

@mcoker wfm!

@mcoker mcoker merged commit 463df45 into patternfly:master May 26, 2021
@patternfly-build
Copy link

🎉 This PR is included in version 4.106.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Rich Helper Text for Form elements
5 participants