-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[UI] Assign links, body and control-label text color to tokens #13630
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
[UI] Assign links, body and control-label text color to tokens #13630
Conversation
LordRembo
left a comment
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.
I have an alternative suggestion: underline the links instead of making them bold.
The most easily reconizable characteristic of an link, is an underline.
Making it bold introduce more ambiguity, because it will just look the same as the table headings.
|
From an Accessibility standpoint, links should not be visual styles similar to other elements in the content/text (like bold or highlight) but visually distinct. Eg. they can be bold or highlighted if you absolutely want them to be, but then they should be designed to look like they are clickable (eg. look like a button or have an icon or add an underline in addition to the bold/highlighted style) |
I know that's the theory but it does feel... weird/uncommon to underline it in this particular use case. I checked a few tools (WordPress, Accountable, Combell, ...) and they all put it in bold with a clear hover. Theory vs habit? |
|
The ideal scenario here would be to keep a normal font weight but apply a saturated color. For the Mautic brand, it could be the yellow or green (everyone would probably find it weird since these colors are rarely used on UI). Is it there a possibility for us to consider? Could I make all links yellow or green? An underline is commonly used for accessibility improvement as a feature which you can enable/disable but it’s not exactly often used on the “normal theme”/for all users… We would also have the same issue since columns header are also a link The point about theory vs habit is important: things become recognizable through repetition. If we keep everything consistent, probably it would be all good to proceed this way |
|
If we can get another review that agrees with just making it bold, I'm fine with it. In the end, consistency and habit are the best indicators if something will work or not. No need to change the colors or styles to something that is not used anywhere. |
|
I like the proposed idea. My only suggestion would be to consider using a slightly lighter font weight for the link probably. Additionally, I would recommend making the links in the 'thead' regular font weight instead of bold, to distinguish them from the links in the list. Other than that, it looks good! 👌 |
|
@code5rick We would fall into the issue mentioned on the feedback, now for thead links, if they become not-bold @RCheesley Any UX idea here? |
|
I'm afraid I'm from the old school ... underlines signify to me that something's a link! 🙈 I like how Joomla does it with their accessibility plugin which ships with core and can be turned on, it gives the user the power to change links to be underlined and a bunch more stuff based on their own needs. You can see it in action on my site here. Could be something to consider in the future, maybe. From their article, it states:
I think making them bold does help if we don't want to underline. I don't think yellow/green would help as those colours don't go well on a white background, but maybe a hover of those colours (probs the Mautic yellow) might assist with signifying a clickable link, if we really want to avoid underlining. |
|
@code5rick 300 (light) I can set 600 to be a bit thicker, but should it be applied to all links on the interface or only to the ones on the second column of all tables? |
|
@RCheesley It would be good if we had a feature like this (lots of enable/disable for each accessibility item) If hard to implement, a high contrast theme with standard accessibility changes would also work fine (kind of a LESS file where accessibility features overwrite the normal theme rules) Regarding the link on tables, this one is hard. Could you select 1 way to proceed, then we start a slack discussion to decide if we implement it or not? The options:
(an additional comment: is this comparison fair? Is it right to compare accessible vs non-accessible? If not, would bold be the only option? Should we give up on this change for now?) |
|
So, these are the options I think we're considering here: Personally, I don't think that making it bold gives enough of direction to the user that it's a click-able link. I think that regular underline (without bold) would be the best option for making it clear it's a link, but it doesn't look so 'tidy'. So I would indeed put this out for discussion in the community and get wider viewpoints. |
revert Revert "fixing broken darken/lighten" This reverts commit bcf4ac98e07c0c17162a74794a8161d5398c3d7c.
|
@andersonjeccel there's a conflict now. |
…e-that-you-dont-know-are-links-is-one-example
|
@escopecz Conflicts solved! |
…e-that-you-dont-know-are-links-is-one-example
…e-that-you-dont-know-are-links-is-one-example
|
@RCheesley @kuzmany @LordRembo could you please give this PR another try? Anderson made more changes according to your feedback. I don't want to get this PR merged until the majority agrees. At the same time, this PR is blocking a bunch of others so we are stuck without your feedback. |
RCheesley
left a comment
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.
Thanks for the nudge @escopecz - @andersonjeccel I think there's a discrepancy with the contact list, because the blue rectangle doesn't show when you click: https://app.screencastify.com/v3/watch/YWqdjuaHnz4krL9k2QOl.
Is this what I'm meant to be testing? I wasn't fully sure from the description and couldn't see massive differences between 5.x and this PR.
|
@RCheesley Thanks for taking time to review! Hm, we have a difference between the way different page lists were created over the time I can take a note to fix this with other PR specifically for minor fixes like HTML structural changes, but it would be great to move this PR here forward to avoid text contrast issues with the light gray tile components 🙏 |
|
OK @andersonjeccel perhaps you could be really specific then on what we should be reviewing? As I wasn't really sure from the original description / conversations in this PR what was and wasn't to be reviewed and what was/wasn't being changed specifically in this PR. |
|
@RCheesley You should review:
|
…e-that-you-dont-know-are-links-is-one-example
…nks-in-small-grey-typeface-that-you-dont-know-are-links-is-one-example
…nks-in-small-grey-typeface-that-you-dont-know-are-links-is-one-example
|
I reverted the outline to ensure this PR will move forward, it's blocking great improvements like |
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.
Right, yeah, I'm fine with this. Sorry, thought I had already re-approved it.
Description:
Edit
Since we didn't get to a conclusion and here [UI] Accessibility features in user profile I add the underlined links as a feature, I used this PR to just assign the links and body text color to the right tokens.
Contrast Ratio goes from 9.62:1 to 18.1:1 for text color. Both passed AAA WCAG guidelines, but the new darker color feels like a good improvement for the eyes
Links remained the same for now...
Before:
After:
Steps to test this PR: