chore(HelperText): updated how status is conveyed - #7751
Conversation
|
Preview: https://pf-pr-7751.surge.sh A11y report: https://pf-pr-7751-a11y.surge.sh |
mcoker
left a comment
There was a problem hiding this comment.
The list below is all the places I spotted something off. It looks like a lot of the issues have basic helper text (no status, not indeterminate). Instead of requiring --HasNoIcon to omit the icon, could you change this block and instead of unless --HasNoIcon, do something like {{#ifAny helper-text-item--IsSuccess, helper-text-item--IsError, helper-text-item--IsWarning, helper-text-item--IsIndeterminate, helper-text-item-icon--type}}? Then you can also just remove the test for @partial-block in this file - that shouldn't be needed.
patternfly/src/patternfly/components/HelperText/helper-text-item.hbs
Lines 15 to 17 in dd7ee44
- There is some weird nested bug on this toolbar demo - https://pf-pr-7751.surge.sh/components/toolbar/html-demos/toolbar-with-validation-on-desktop/
- File upload example needs
--HasNoIcon- - Progress example needs
--HasNoIcon-patternfly/src/patternfly/components/Progress/examples/Progress.md
Lines 255 to 264 in dd7ee44
- First form field in this helper text demo needs
--HasNoIcon- - All of the
{{> form-helper-text}}partials in this form demo need--HasNoIcon- https://github.com/patternfly/patternfly/blob/dd7ee4464dba7228fc3cba7ff665996287d8835e/src/patternfly/demos/Form/examples/BasicForms.md?plain=1 - Default helper text under the "dynamic" section has an icon on https://www.patternfly.org/components/helper-text/html#dynamic, but not on https://pf-pr-7751.surge.sh/components/helper-text#dynamic - was that intentional? Seems to me like it should have the icon but I'm not 100%.
- Default helper text in this form help text example has the indeterminate icon https://www.patternfly.org/components/forms/form/html#help-text, but this doesn't https://pf-pr-7751.surge.sh/components/forms/form#help-text. That seems like a good change to me, but just wanted to make sure it was intentional? FWIW it isn't clear in Figma - I see "default" with and without the hyphen/indeterminate icon.


| {{{helper-text-item-icon--attribute}}} | ||
| {{/if}}> | ||
| {{#if helper-text-item--HasIcon}} | ||
| {{#if helper-text-item--IsIndeterminate}} |
There was a problem hiding this comment.
Can you fix the indentation of this block?
dd7ee44 to
4f09462
Compare
|
@mcoker let me know if I missed anything - I updated the unless block to that ifAny and removed instances of the HasNoIcon hbs attribute. For your last 2 bullets about the default icon, at least going by React logic we don't allow an icon for default helper text unless the |
mcoker
left a comment
There was a problem hiding this comment.
Looks awesome! Only question is around removing the "-" from the basic helper text in the dynamic example - want to make sure that was intentional, and if so that design approves. I still see the icon in the helper text stuff in figma (under forms) but it isn't clear if/when that's needed.
|
@mcoker fwiw this is the React PR that had made Penta updates and removed an icon for a |
|
@lboehling do you know if the "-" before the default helper text in "dynamic" helper text is supposed to be there or not? I always assumed it was there for non-status helper text so that as the helper text updates as you type, it doesn't move around too much since all variations of helper text would have some kind of icon. Is that true or should our dynamic example not include the "-" before the regular helper text? https://www.patternfly.org/components/helper-text/html#dynamic
|
I don't know the background for this, but I assumed the same as you did -- that it was there to keep the helper text from jumping around if a status was added to it when placed in a list of requirements/helper texts (like in password strength). I don't think it's needed for all helper text w/o status, just in the dynamic option with a list of requirements? |
|
🎉 This PR is included in version 6.3.0-prerelease.69 🎉 The release is available on: Your semantic-release bot 📦🚀 |



Closes #7282