Skip to content

chore(HelperText): updated how status is conveyed - #7751

Merged
mcoker merged 2 commits into
patternfly:mainfrom
thatblindgeye:iss7282_helperTextStatusConvey
Sep 12, 2025
Merged

mcoker merged 2 commits into
patternfly:mainfrom
thatblindgeye:iss7282_helperTextStatusConvey

Conversation

@thatblindgeye

Copy link
Copy Markdown
Contributor

Closes #7282

  • Updated some handlebars logic so that you shouldn't opt into an icon being rendered, rather it's the default for status helper text items.
  • For the SR text, I went with something different than what React has: rather than appending "[status] Status" after the visible helper text, I prepended it with "[status]:". In React we currently allow a way to omit the SR text per a product ask at one point, and in the future it may make sense to allow more flexibility with how the SR text is rendered in React (instead of always rendering it at the end of a helper text item, allow consumers to have it at the beginning). I can update Core to match React for now, though, if we'd prefer that.

@thatblindgeye
thatblindgeye requested review from a team, mcoker and nicolethoen and removed request for a team August 21, 2025 13:14
@patternfly-build

patternfly-build commented Aug 21, 2025 •

Copy link
Copy Markdown
Collaborator

@mcoker mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

{{#unless helper-text-item--HasNoIcon}}
{{> helper-text-item-icon}}
{{/unless}}

{{{helper-text-item-icon--attribute}}}
{{/if}}>
{{#if helper-text-item--HasIcon}}
{{#if helper-text-item--IsIndeterminate}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you fix the indentation of this block?

@thatblindgeye
thatblindgeye force-pushed the iss7282_helperTextStatusConvey branch from dd7ee44 to 4f09462 Compare August 29, 2025 12:25
@thatblindgeye

Copy link
Copy Markdown
Contributor Author

@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 icon prop is passed in. We could add a Core example showing "custom icons" similar to what React has if we'd want some documentation that states "hey you can add an icon to default helper text, just make sure it isn't the same as another helper text variant/is consistent across your app"

@thatblindgeye
thatblindgeye requested a review from mcoker September 3, 2025 19:07

@mcoker mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Image

@thatblindgeye

Copy link
Copy Markdown
Contributor Author

@mcoker fwiw this is the React PR that had made Penta updates and removed an icon for a default variant: patternfly/patternfly-react#10029

@mcoker

mcoker commented Sep 11, 2025

Copy link
Copy Markdown
Contributor

@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

Screenshot 2025-09-11 at 2 43 56 PM

@lboehling

Copy link
Copy Markdown

@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

Screenshot 2025-09-11 at 2 43 56 PM

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?

@mcoker mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified offline that "regular" help text in a dynamic scenario should not have an icon, but intermediate text should. Here's a screenshot of what that would look like if both were present.

Image

@mcoker
mcoker merged commit bfb5ea0 into patternfly:main Sep 12, 2025
4 checks passed
@patternfly-build

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 6.3.0-prerelease.69 🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HelperText - lack of conveying status accurately

5 participants