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
feat: add underline prop to hyperlink #739
feat: add underline prop to hyperlink #739
Conversation
icons/jsx/SpinnerSimple.jsx
Outdated
@@ -8,6 +8,7 @@ function SvgSpinnerSimple(props) { | |||
viewBox="0 0 24 24" | |||
fill="none" | |||
xmlns="http://www.w3.org/2000/svg" | |||
aria-label="loading" |
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.
These files are generated by code. If you want to specify a label on the loading icon you need to do it in your application. Paragon doesn't have localization built in yet.
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.
Currently if I don't provide any icon to Stateful button, it uses a default spinner. Is there a way to add aria-label to that?
Or will we need to explicitly add icon to stateful button and then add aria-label to it?
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.
We could hardcode English in Paragon for now, but if you do add your label here please: https://github.com/edx/paragon/blob/master/src/StatefulButton/index.jsx#L51
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.
@abutterworth I made the changes you suggested ^ but this didn't fix the accessibility problem because the icon is not focusable for stateful buttons (running ARC toolkit gave error that I should not add aria-label to it). I have taken another route to achieving the same thing. Kindly see changes to StatefulButton component.
Here is the Design Documentation for links: https://www.figma.com/file/eGmDp94FlqEr4iSqy1Uc1K/Paragon-2021?node-id=789%3A42 If you need the functionality you've added in this PR please take this path:
The props you add to hyperlink should add or remove css classes for styling links. Some of which we already have: https://github.com/edx/paragon/blob/master/scss/core/_typography.scss#L39 What's here probably needs some expanding or reworking. Here's a rough idea of what styles could become:
Since you're PR relates only to underlining you could choose to add just Let me know if you have any questions or concerns. |
3e2e8e1
to
68ef8a3
Compare
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.
See my comment above
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.
It looks good.
Could you add examples of the different variants and isInline in the documentation site? This can be done by editing this README file: https://github.com/edx/paragon/blob/master/src/Hyperlink/README.md
b753032
to
4a504ef
Compare
🎉 This PR is included in version 14.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changes:
-- Some designs require no underline under hyperlink component.
-- This is added to for accessibility because sometime stateful button has a pending state that doesn't have a text and only spinner is shown.