-
Notifications
You must be signed in to change notification settings - Fork 29
feat(Status): Add Status component #137
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
Conversation
<Flex title={label} alignItems={{ default: 'alignItemsCenter' }} {...props}> | ||
{ icon && ( | ||
<FlexItem className={classes.icon}> | ||
{React.cloneElement(icon, { className: clsx('pf-v5-u-mr-md', icon?.props?.className), title: label, 'data-ouia-component-id': `${ouiaId}-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.
I would recommend that the icon be wrapped in a PF Icon component and use the status
prop on that component to control color rather than hardcoding a color in the passed icon.
{ !iconOnly && ( | ||
<FlexItem> | ||
<Text ouiaId={`${ouiaId}-label`} className={classes.statusLabel}>{label}</Text> | ||
<Text component={TextVariants.small} ouiaId={`${ouiaId}-description`} className={classes.statusDescription}>{description}</Text> |
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.
This should only be rendered if there is a description to display.
<Flex title={label} alignItems={{ default: 'alignItemsCenter' }} {...props}> | ||
{ icon && ( | ||
<FlexItem className={classes.icon}> | ||
{React.cloneElement(icon, { className: clsx('pf-v5-u-mr-md', icon?.props?.className), title: label, 'data-ouia-component-id': `${ouiaId}-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.
By reusing the label as the icon's title, it causes the screen reader to repeat itself when it reads the icon and then reads the label.
And since the label is marked as optional, when people don't want to display a label, they may leave it blank and leave the icon with no title - thus leaving it inaccessible. I wonder if the icon's title should be an additional field which defaults to the value of the status
passed to the Icon
component? and the status should be a required field.
Then it should be overridden and given a more descriptive title when using the iconOnly
prop. Maybe we could print a message in the console if someone uses iconOnly
and then doesn't provide a descriptive Icon title.
@nicolethoen thank you for reviewing and great comments! All should be addressed now |
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.
This LGTM 👍🏻
🎉 This PR is included in version 5.3.0-prerelease.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
finishing #125
closes #94
RHCLOUD-30295