-
Notifications
You must be signed in to change notification settings - Fork 30
feat(CullingInfo): add culling info component #568
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
|
|
||
| const useStyles = createUseStyles({ | ||
| inventoryCullingWarning: { | ||
| color: 'var(--pf-v6-global--warning-color--200)', |
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.
In general, we should avoid using tokens which end with a number - those are called base tokens. Instead we should always be using semantic tokens which will adjust automatically in dark theme and be more context aware.
In this case, I believe the correct variable is --pf-t--global--icon--color--status--warning--default
And for the danger variant, it should be --pf-t--global--icon--color--status--danger--default
if those colors look incorrect, we could consult @kaylachumley for different tokens
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 replaced them with semantic tokens
|
@InsaneZein can you please add some examples showing different color variants of the component? (similar to screens in the JIRA). Ideally dynamically derived from the current date so they look always the same |
...y-docs/content/extensions/component-groups/examples/CullingInfo/CullingInfoCustomExample.tsx
Outdated
Show resolved
Hide resolved
|
@fhlavac fixed coloring and added example for both warning and danger versions |
| return ( | ||
| <React.Fragment> | ||
| <Tooltip {...props} content={msg} position="bottom"> | ||
| <span |
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.
From an accessibility perspective, tooltips need to be placed on interactable elements, such as links or buttons in order for them to able to be accessed by keyboard and screen readers.
Could these components be placed in a plain button, rather than a span?
<Button variant="plain" aria-label="Action" icon={<TimesIcon />} />
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.
Can these buttons be inside a span or div? Or would that still get in the way of screen readers?
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.
The button could be anywhere! The key is just that there needs to be an element which can take browser focus and has some sort of affordance which cues the user that there is more information available if they interact with this icon.
| marginRight: 'var(--pf-t--global--spacer--sm)' | ||
| }, | ||
| messageFont: { | ||
| fontWeight: 'var(--pf-t--global--font--weight--200)', |
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.
rather than a base font weight token, let's use the semantic token --pf-t--global--font--weight--body--bold
|
@edonehoo when you have time, can you please check the docs updates and overall wording? Thank you |
| > | ||
| {isError && <ExclamationCircleIcon />} | ||
| {isWarn && <ExclamationTriangleIcon />} | ||
| {children} |
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.
The first example is still not screen reader accessible - and it may be true that the button needs to replace the span - or the Tooltip needs to be placed on the button within the span.
you can see that the first example is screen reader inaccessible in the video below. you can also see how the tooltip is not being triggered by the screenreader since I think the tooltip needs to be placed on the button itself rather than the span outside the button.
i recommend testing the examples by tabbing through them as a sanity check - if they are keyboard accessible, it's much more likely that it'll be screen reader accessible.
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.
oops I forgot to add the buttons to the tooltip version of the CullingInfo. I was able to tab through them after the fix.
| subsection: Status and state indicators | ||
| # Sidenav secondary level section | ||
| # should be the same for all markdown files | ||
| id: Culling information |
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.
| id: Culling information | |
| id: Status timestamp |
Previously, (via this Miro), we considered renaming "culling info" to be "status timestamp" for easier understanding. I don't see a timestamp in the examples, though, so maybe the design has changed. Do we still want to do that?
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.
Another option could be "Stale warning", "Expiration warning", "Stale data warning"
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'm not able to access the Miro link, but there isn't a timestamp. It is either a warning/danger tooltip or a warning/danger icon with a custom message. Is there a naming option you would prefer?
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.
Okay, let's go with "stale data warning" if that sounds okay with you? I also don't mind sticking with "culling" if that's more accurate, but if so I would suggest "culling warning" instead of "culling information"
|
|
||
| import CullingInformation from '@patternfly/react-component-groups/dist/dynamic/CullingInfo'; | ||
|
|
||
| A **culling information** component displays a warning for when an object will become culled or stale. It can display this as a tooltip or 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.
| A **culling information** component displays a warning for when an object will become culled or stale. It can display this as a tooltip or text. | |
| A **culling information** component displays a warning status when an object is stale and planned for removal. Additional warning details can be displayed a tooltip or text label. |
if we change the component name, update here as well
|
|
||
| ### Basic culling information | ||
|
|
||
| A basic culling information example |
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.
| A basic culling information example | |
| A basic culling information component displays a warning icon with additional details in a tooltip, including a timeline for data removal. |
|
|
||
| ### Culling information with customized props | ||
|
|
||
| For further customization, you can choose to render the tooltip as a message beside the icon instead. And you can utilize all properties of the [Tooltip component](/components/tooltip), with the exception of `content`. |
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.
| For further customization, you can choose to render the tooltip as a message beside the icon instead. And you can utilize all properties of the [Tooltip component](/components/tooltip), with the exception of `content`. | |
| Instead of sharing details in a tooltip, you can place a short message beside the icon instead. You can still utilize all properties of the PatternFly [tooltip component](/components/tooltip), with the exception of `content`. |
| currDate={new Date()} | ||
| culled={new Date('Fri Feb 07 2024')} | ||
| staleWarning={new Date('Mon Feb 03 2024')} | ||
| render={() => (<React.Fragment>This is an error message. Item is past due</React.Fragment>)}> |
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.
| render={() => (<React.Fragment>This is an error message. Item is past due</React.Fragment>)}> | |
| render={() => (<React.Fragment>This is an error message where the item is overdue</React.Fragment>)}> |
* trying some things * some suggestions
|
@edonehoo @nicolethoen I've renamed CullingInfo to StaleDataWarning |
edonehoo
left a comment
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.
a couple of places to update capitalization, but other than that this looks good!
| subsection: Status and state indicators | ||
| # Sidenav secondary level section | ||
| # should be the same for all markdown files | ||
| id: Stale Data Warning |
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.
| id: Stale Data Warning | |
| id: Stale data warning |
|
|
||
| import StaleDataWarning from '@patternfly/react-component-groups/dist/dynamic/StaleDataWarning'; | ||
|
|
||
| A **Stale Data Warning** component displays a warning status when an object is stale and planned for removal. Additional warning details can be displayed as a tooltip or text label. |
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.
| A **Stale Data Warning** component displays a warning status when an object is stale and planned for removal. Additional warning details can be displayed as a tooltip or text label. | |
| A **stale data warning** component displays a warning status when an object is stale and planned for removal. Additional warning details can be displayed as a tooltip or text label. |
|
|
||
| ### Stale data warning with customized props | ||
|
|
||
| Instead of sharing details in a tooltip, you can place a short message beside the icon. You can still utilize all properties of the [Tooltip component](/components/tooltip), with the exception of `content`. |
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.
| Instead of sharing details in a tooltip, you can place a short message beside the icon. You can still utilize all properties of the [Tooltip component](/components/tooltip), with the exception of `content`. | |
| Instead of sharing details in a tooltip, you can place a short message beside the icon. You can still utilize all properties of the [tooltip component](/components/tooltip), with the exception of `content`. |
|
I'll rereview this once surge starts working for me again... 😞 |
nicolethoen
left a comment
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 looks great! thanks for working through all the feedback! 🍕 🎈
|
🎉 This PR is included in version 6.2.0-prerelease.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |



For RHCLOUD-33371. This adds the culling info component