-
Notifications
You must be signed in to change notification settings - Fork 373
feat(Progress): add hideStatusIcon flag #12038
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
base: main
Are you sure you want to change the base?
feat(Progress): add hideStatusIcon flag #12038
Conversation
|
Preview: https://pf-react-pr-12038.surge.sh A11y report: https://pf-react-pr-12038-a11y.surge.sh |
Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com>
e86cf84 to
6c5dcc6
Compare
| )} | ||
| {StatusIcon && ( | ||
| <span className={css(progressStyle.progressStatusIcon)}> | ||
| <span className={css(progressStyle.progressStatusIcon)} data-testid="progress-status-icon"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to hard code this test id into the component? I'm not sure what a better way to do it would be, but I don't love this, and the fact that there isn't a better way is probably signaling a lack of a11y on this. Would love @thatblindgeye's thoughts on this.
Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add a demo? Probably want to add a prop comment. Otherwise looks fine to me!
Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com>
Co-authored-by: Rebecca Alpert <ralpert@redhat.com>
|
@rebeccaalpert i replaced a few examples with an interactive one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Surge doesn't have it for whatever Surge-y reason, but localhost looks great.
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned in a Slack convo and just commenting here for future reference, but regarding a potential implementation where a consumer wants no icon + no measure + status colors, we could probably recommend:
- if the status color isn't critical and is really more visual in nature (e.g. maybe there's no actual error or warning message or anything), then the progress bar itself would probably suffice as a means to convey whatever message is meant to be conveyed (e.g. "This thing is almost full")
- Otherwise, strongly recommened an icon be used, or at the very least a visible measure value if perhaps helper text is provided that has its own error icons.

Closes #11983