Conversation
Signed-off-by: ediiotero <e.otero81@gmail.com>
jamigibbs
left a comment
There was a problem hiding this comment.
@edmondpewing Is this accessibility and architecture outline still accurate for the development of this? If not, could you note what has changed? That will help us with our reviews.
It would also be helpful to link to the Figma designs.
jamigibbs
left a comment
There was a problem hiding this comment.
Thanks for working on this Eddie! I've left a few comments/questions but mostly I think we just need to make sure we're developing for the requirements in the accessibility review doc linked above. That might mean creating a separate component using va-card as a child but I'll leave that up to you to determine.
Signed-off-by: ediiotero <e.otero81@gmail.com>
| <footer> | ||
| {linkHref && linkText ? ( | ||
| <va-link | ||
| aria-describedby={ariaDescribedbyIds} |
There was a problem hiding this comment.
Thanks. I missed that. Its changed now.
There was a problem hiding this comment.
Hi @ediiotero! Do you mind updating the description of this PR so that it more accurately reflects the changes made since switching from a va-card update to a new va-card-status component?
Also can you confirm that we want all of those different Storybook examples instead of just error? I was thinking that this component would only have an error state/status. Maybe in the future there would be other statuses but I didn't think it would yet. I could have missed some detail somewhere though.
I will hold off on doing a code review until those things are clarified.
Error Status in Storybook
amyleadem
left a comment
There was a problem hiding this comment.
Hi @ediiotero, thanks for all your work on this. I did a preliminary review and noticed that there were some opportunities to simplify this implementation by removing some slot support. Can you take a look at the comments and let me know if you have any questions or concerns?
I can come back for a deeper review after these updates.
|
All requested changes complete. I'm including this link to the Figma file I used while working on this. It might be helpful in understanding some of the decisions I made. As an FYI the Figma is outdated now: Card-Status Figma |
There was a problem hiding this comment.
@ediiotero I have some more testing to do, but wanted to add a few more comments with some issues I found. Let me know if you have questions!
Signed-off-by: ediiotero <e.otero81@gmail.com>
There was a problem hiding this comment.
LGTM!
I reviewed the component for accessibility by checking the component in the following environments:
- MacOS/VoiceOver/Safari
- MacOS/VoiceOverChrome
- iOS/VoiceOver/Safari
- Windows/NVDA/Chrome
- Windows/JAWS/Chrome
- Android/Talkback
Note: On my Pixel device, I did hear some unexpected announcements when swiping through the content, but I think it was an anomaly. It would occasionally announce "start 17px color black background color white font family source sans pro" when swiping to the telphone number. However, I did not see this behavior before today, there is nothing in the code that would indicate that this should be read out, and we were not able to replicate this behavior in other devices/emulators. Additionally, the issue was happening in other stable components on the failing device. All of this points to it being a strange glitch, but interested to learn if other discover this.Update: This was related to a recent settings update in Talkback. The style announcements stopped once I updated the text styling announcement settings (Settings > Accessibility > TalkBack > Settings > Verbosity > Speak text formatting). This was not caused by the component.
I completed the tests in the component's accessibility checklist, which included testing for zoom, keyboard, font resizing, forced colors/high contrast mode, and visual design cues like color contrast.
Before merge, let's get another engineering review and design/content review (@jeana-adhoc ?).
RyanMunsch
left a comment
There was a problem hiding this comment.
LGTM. Nice work, @ediiotero!
| /** | ||
| * @componentName Card Status | ||
| * @maturityCategory use | ||
| * @maturityLevel deployed |
There was a problem hiding this comment.
Is this the right maturity level we should be using? For new components, should we be using "Available". After this is used on production in 3x products looks like we can promote it to deployed
There was a problem hiding this comment.
I'm not sure. I was following the Card component @amyleadem Do you know what the maturityLevel should be?
There was a problem hiding this comment.
@ediiotero New components should be "Use with caution: Candidate". For example, va-button-icon:
jeana-adhoc
left a comment
There was a problem hiding this comment.
LGTM! Thanks for all of your work on this.

Chromatic
https://4519-Card-status-comp--65a6e2ed2314f7b8f98609d8.chromatic.com
Description
Adding a new
va-card-statuscomponent. It usesva-cardas a child and includes props for heading, footer links, and tag status.Related tickets and links
Closes 4519
Screenshots
Testing and review
Running unit test and manual testing within the story.
Approvals
See the QA Checklists section below for suggested approvals. Use your best judgment if additional reviews are needed. When in doubt, request a review.
Approval groups
Add approval groups to the PR as needed:
QA checklists
Use the QA checklists below as guides, not rules. Not all checklists will apply to every PR but there could be some overlap.
In all scenarios, changes should be fully tested by the author and verified by the reviewer(s); functionality, responsiveness, etc.
✨ New Component Added
minorlabel🌱 New Component Variation Added
minorlabel🐞 Component Fix
patchlabel♿️ Component Fix - Accessibility
patchlabel🚨 Component Fix - Breaking API Change
majorlabel🔧 Component Update - Non-Breaking API Change
minorlabel📖 Storybook Update
ignore-for-releaselabel🎨 CSS-Library Update
css-librarylabel