Skip to content
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

buttons with text-decoration set to none get underline for text decoration. #36644

Closed
ahmaxed opened this issue Aug 20, 2019 · 11 comments · Fixed by #36662
Closed

buttons with text-decoration set to none get underline for text decoration. #36644

ahmaxed opened this issue Aug 20, 2019 · 11 comments · Fixed by #36662
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: UI Threads for addressing UX changes and improvements to user interface status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.

Comments

@ahmaxed
Copy link
Member

ahmaxed commented Aug 20, 2019

On the profile page, displayed button with text-decoration set to none gets an underline most likely due to the styles of its parent (a tag)

image

same issue also present in the current welcome page.

image

@ahmaxed ahmaxed added scope: UI Threads for addressing UX changes and improvements to user interface platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. release: next/beta labels Aug 20, 2019
@huyenltnguyen
Copy link
Member

@ahmadabdolsaheb So we are just going to set text-decoration: none to the a tag then? Or is there another approach you prefer?

Btw, I noticed that the word Challenges in Take me to the Challenges is capitalized, is this a typo or it's on purpose?

@ahmaxed
Copy link
Member Author

ahmaxed commented Aug 21, 2019

@huyenltnguyen the a tags are to underlined all across. the only exceptions are navigation, footer, buttons, and a couple of more places.

@moT01, thank you for tackling this. is there are a way we could resolve this globally rather than adding classes to a tags one by one?

@moT01
Copy link
Member

moT01 commented Aug 21, 2019

I found that an underline is applied globally - We could just change that, but it would remove the underlines in places where we probably want them. There's one in the bottom paragraph of the homepage, for instance. A few more on the learn page.

@ahmaxed
Copy link
Member Author

ahmaxed commented Aug 21, 2019

interesting, wouldn't class styles override global styles? it does for everything else.

@ahmaxed ahmaxed closed this as completed Aug 21, 2019
@ahmaxed ahmaxed reopened this Aug 21, 2019
@ahmaxed
Copy link
Member Author

ahmaxed commented Aug 22, 2019

@moT01
if a global solution is not possible we can perhaps define a class globally for removing the underline and give it to every a tag that we want to remove the underline from.

@ojeytonwilliams
Copy link
Contributor

@ahmadabdolsaheb @moT01 it's all a bit weird. The underlined links are the ones which have a button inside an a. In this case the text-decoration is being inherited from the a and so defaults to underline completely ignoring the button style. It's a quirk of css that I don't fully understand, but it is what it is.

I think the right approach is just to remove the buttons, since they should not go inside a elements anyway - see this SO discussion.

@ahmaxed
Copy link
Member Author

ahmaxed commented Aug 22, 2019

@ojeytonwilliams not sure why we do it this way.
Any thoughts @ValeraS?

@ahmaxed
Copy link
Member Author

ahmaxed commented Aug 22, 2019

We might have buttons around for a11y reasons.
should we give the link to the formaction property of the button so it acts like a a tag?

@ojeytonwilliams
Copy link
Contributor

As far as accessibility goes, I think the main thing is that links are obviously links and vice versa. If a link looks like a button, that could potentially cause confusion, but I think it's clear what's going on in this case.

@moT01
Copy link
Member

moT01 commented Aug 27, 2019

So, is it fine to remove the <Button> elements? That's the solution that seems to be the best after looking at this - unless they are there for some specific reason.

@moT01 moT01 added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Aug 27, 2019
@ahmaxed
Copy link
Member Author

ahmaxed commented Aug 27, 2019

@moT01 yes, we should remove the <Button> when linking to other pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: UI Threads for addressing UX changes and improvements to user interface status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants