feat(EmptyDetailsView): add component #65
Conversation
There was a problem hiding this comment.
It's hard for me to gage how inconvenient it'll be that the test-id is not exposed any more. Once they migrate to PF6, they will have to cross that bridge themself if they decide it matters, so for now, I think it's okay to not worry about that one item in the hidden component.
| /** Source path to an icon image. */ | ||
| iconImage?: string; | ||
| /** Alternative text for icon image if image can't load. */ | ||
| imageAlt?: string; |
There was a problem hiding this comment.
Do we need alt text for this image? This image is purely decorative and doesn't provide any additional meaning to the user that isn't already provided with the contents on the page.
There was a problem hiding this comment.
The prop must stay there so the interface is the same as in the original component (https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/components/EmptyDetailsView.tsx), it was an agreement with Jeff that we don't change it so the migration is easy. But it is a good point and something to potentially enhance in the future
There was a problem hiding this comment.
Thanks for the explanation. That makes sense. I think we can keep the prop, but my suggestion would be to set the example to have the following value:
imageAlt=""
And also modify the code so that this results in the following html:
alt="" (vs just the alt that results from having an empty prop value)
You can see more details about this practice for decorative images here: https://www.w3.org/WAI/tutorials/images/decorative/
There was a problem hiding this comment.
That makes perfect sense. Also can we overwrite the prop value to always result in alt="" even if consumer provides an alt text? (so we basically remove the ability to set an alt text, without actually removing the prop)
There was a problem hiding this comment.
I'm not sure about always overwriting the prop value to result in alt="", and would want @nicolethoen or @jeff-phillips-18 to comment on whether we always ignore values specified in the imageAlt prop. Our usage of these images is decorative, and don't provide meaningful contents to the user, so we would want our examples to reflect that.
But I'm not sure where these components would be consumed, and there's always a possibility that a consumer would choose to use an image that does provide meaningful contents, for which they would need a description.
| /** Button which initiates the creation. */ | ||
| createButton?: React.ReactNode; | ||
| /** Extra children to render inside EmptyStateFooter. */ | ||
| footerExtraChildren?: React.ReactNode; |
There was a problem hiding this comment.
The example provided in the preview shows an empty state with just a single primary action, but it would be good to have an example that shows a secondary action, too.
| title="Start by adding cluster storage" | ||
| description="Cluster storage saves your project’s data on a selected cluster. You can optionally connect cluster storage to a workbench." | ||
| iconImage={clusterImage} | ||
| imageSize="240px" |
There was a problem hiding this comment.
Can we change 240px to 320px
Also, this size is different than the size shown in /EmptyDetailsView/EmptyDetailsView.tsx below, which is using 320px.
We have new guidelines for empty states that recommend using 320px for illustrations. It would be ideal if our examples in the preview use the values we are recommending.
|
Overall this looks good. I did have a few requests regarding the preview examples, and one question about image alt text that would be good to get someone else's opinion on. |
- using "type" with Omit instead of "interface" doesn't work for some reason with docs-framework
adamviktora
force-pushed
the
EmptyDetailsView
branch
from
9d0fcd6 to
5c3f8aa
Compare
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #26
direct link to new docs: https://ai-infra-ui-components-pr-65.surge.sh/ai-infra-ui-components/emptydetailsview
Issues:
EmptyStateHeader(it is now an internal component in Patternfly), thusdata-testid="empty-state-title"must be removed, might be worth opening an issue in pf-react?