-
Notifications
You must be signed in to change notification settings - Fork 12
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
va-on-this-page: Rewrite to use ul instead of dl for list of links #1346
Conversation
))} | ||
</dd> | ||
</dl> | ||
</li> |
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.
Are there tests that this markup change will affect in vets-website?
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.
There is one test that looks for this component: vets-website/src/applications/sco/tests/accessibility.cypress.spec.js, but the test is structured in such a way that it should still work with this re-write.
It's worth noting that there are currently 5 pages that have "fake component" versions of the va-on-this-page that all still use the dl/dd combo to display links and should probably be converted:
- vets-website/src/applications/combined-debt-portal/debt-letters/components/DebtDetailsOnThisPageLinks.jsx
- vets-website/src/applications/combined-debt-portal/debt-letters/components/OnThisPageLinks.jsx
- vets-website/src/applications/combined-debt-portal/medical-copays/components/OnThisPageDetails.jsx
- vets-website/src/applications/combined-debt-portal/medical-copays/components/OnThisPageOverview.jsx
- vets-website/src/applications/combined-debt-portal/medical-copays/components/OnThisPageStatements.jsx
I'll write these up in a separate ticket for after this component ships.
@Andrew565 If the link wraps, can we have the second line align with the first like it is in "before"? |
@danbrady Ready for re-review, updated the "after" screenshot at the top. |
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.
LGTM! (And thanks for fixing the vertical alignment of the icon too!)
padding: 0.5em; | ||
max-width: 372px; |
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.
@Andrew565 Would you be able to revert the width back to the original? My apologies for adding that feedback to this ticket. I talked to Carol about the issue she was able to move the ticket (#951) to address the width to Sprint 13 so we can address the width then.
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.
Done
Chromatic
https://1998-on-this-page-rewrite--65a6e2ed2314f7b8f98609d8.chromatic.com
Description
Closes #1998
QA Checklist
Screenshots
Before:
After:
Acceptance criteria
Definition of done