Migrate SimpleTable.tsx to chakra
#8744
Conversation
amit-ksh
requested review from
samajammin,
wackerow,
corwintines,
pettinarip,
minimalsm and
jmcook1186
as code owners
✅ ethereum-org-website-dev deploy preview ready
|
There was a problem hiding this comment.
Hey @amit-ksh! 👋🏼
I have a couple of suggestions to instantly make this more accessible with external links. 😄 (Using tables is an instant boost!
If we keep where the Link component is used for the row, then you will have an anchor tag rendered, wrapping td tags... which is not really approved in HTML styling validation, and might create issues with screen readers.
With the original intention of the whole row being clickable, the LinkOverlay component tandem is a great approach!
@pettinarip I would like your thoughts here. 🤘🏼
|
Made all the changes as you said @TylerAPfledderer. |
Co-authored-by: Pablo Pettinari <pettinarip@gmail.com>
Co-authored-by: Pablo Pettinari <pettinarip@gmail.com>
|
Made all the changes as you suggested @pettinarip |
There was a problem hiding this comment.
@amit-ksh and @pettinarip: I've provided additional thoughts. :)
| }} | ||
| isExternal | ||
| > | ||
| {name} |
There was a problem hiding this comment.
This is a personal recommendation, as it is a minor addition for A11y. (For @pettinarip approval 😄 )
Include visually hidden text for the LinkOverlay render only which reads (Opens a new window) (with the parens) since all the link that are ever passed into this table are assumed to be external and the screen-reader user should be notified of that.
Maybe should have this under a translation function too and supplied in the
intl/en/common.jsonfile
| {name} | |
| {name} | |
| <VisuallyHidden> | |
| <Translation id="sr-link-opens-new-window" /> | |
| </VisuallyHidden> |
And the json add:
"sr-link-opens-new-window": "(Opens a new window)"
There was a problem hiding this comment.
Hey @TylerAPfledderer, should I make these changes or wait for @pettinarip approval?
There was a problem hiding this comment.
@amit-ksh I would wait to make sure it's OK since it is adding to the structure.
There was a problem hiding this comment.
I think this is a worthy discussion we can have with the rest of the team on a different issue since I think that if we apply that change we should do the same with the rest of the external links we have on the site, to keep consistency. For that reason, I don't see that change as part of the scope of this PR/migration.
Happy to see this issue created and discussed with more people. I haven't seen this pattern a lot on other sites. I'm curious to hear what others think about it.
Co-authored-by: Tyler Pfledderer <tyler.pfledderer@gmail.com>
| }} | ||
| isExternal | ||
| > | ||
| {name} |
There was a problem hiding this comment.
I think this is a worthy discussion we can have with the rest of the team on a different issue since I think that if we apply that change we should do the same with the rest of the external links we have on the site, to keep consistency. For that reason, I don't see that change as part of the scope of this PR/migration.
Happy to see this issue created and discussed with more people. I haven't seen this pattern a lot on other sites. I'm curious to hear what others think about it.
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2023 Ethereum.org Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
Description
Migrate the
SimpleTablecomponent from emotion to chakra using Chakra's Table component.Related Issue
Closes #8638