-
Notifications
You must be signed in to change notification settings - Fork 55
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
Figma update: Add white link variation to va-link #2776
Comments
Hey team! Please add your planning poker estimate with Zenhub @babsdenney @danbrady @LWWright7 |
@danbrady @babsdenney - This one is ready for review! |
@LWWright7 Thanks for taking this on, Lucas! This is looking great. I think we should restructure a bit though, because "reverse" is more of a "variation" (or maybe a "theme") than a "state". In other words, for example, the link can be both "reverse" and have a "hover" state. Or it can be "reverse", "active", and "visited" at the same time. So every combination that currently existed before, needs to be reverses. The end result essentially double the total number of combinations. |
@danbrady ahhhh....I see what you're saying.....it should have been a column instead of a row, basically....right? or do they all need to be created in reverse? |
Yeah, multiple columns though. Think of it this way: The current component has 7 variations, 5 states for single line and 5 states with 2 lines. That's 70 combos. We need all those combos on a reverse background, so another 70 combinations. So, essentially, the whole component is getting duplicated and put on a dark background. (We don't want an actual duplicated component though because we want to handle it all changing properties.) Make sense? |
@danbrady Yep, totally. makes sense One question though....for the variant, would it make sense to just create a boolean value for the reverse variations (ex: Reverse? T/F) or how do you think that should be handled? |
@LWWright7 Hmm... good question. A boolean could work as there are just two "modes". Although, if we ever add did a third mode though that would make updating a bit harder. It might be safer to make it a variant property so it gets a dropdown list that we can add/subtract too as needed. |
@danbrady Good point! I went with "Dark/Light" mode instead of the boolean. One last question...I looked on the VADS Storybook for documentation on the "visited" link for the dark ("reverse") and didn't really see that topic covered or an example of it there. Do you know what it should be? |
@LWWright7 How does the web component currently handle it? I don't think we would want that purple as it would be hard to see on dark backgrounds. (BTW, I think majority of the time dark backgrounds are either "primary dark" or "primary darker".) Maybe run the contrast reqs by Ryan to get an accessibility standpoint? |
@danbrady That's why I ask...the web component doesn't even seem to have a "visited" state...the text stays the same color (white) and nothing changes. It behaves that way in storybook and on va.gov live atm. So I'm guessing that's the expected behavior....so no visited variation/state? |
Yeah, that seems like a potential error to me but maybe accessibility weighed in on that? It's probably better that we find out if that's acceptable or not. |
@rsmithadhoc Would you happen to know the answer to this question? Is this behavior mentioned above normal and expected (that is, no visited state for link in reverse/dark mode?) or is there another color that would meet a11y as the normal state for visited doesn't seem that it would pass contrast testing? |
I think the issue here is it would have to pass WCAG AA on the lightest background that it would be used on. I think that might be the light blue used in the gradient on the hero section on va.gov, To achieve that for normal-sized text, it would need to have a 4.5:1 contrast ratio. If we went with our visited link purple, it would need to be #DBD0E7. The action link had a white link before we updated the standard link and it also doesn't have a visited state. I'm thinking that visited state may have been intentionally kept white for the action link, because the color would have to be pretty close to white anyway, so the contrast between active and visited links would be insufficient (very light purple vs. white). According to WCAG 2.2, setting a link's visited status is not a responsibility of the author. However, if we do, it has to meet contrast ratio requirements. I'm okay with omitting the visited state from this (keep it white), unless we have a suitable color that looks good. |
@babsdenney & @danbrady This one is ready for another look! |
Thanks for tackling all those variations! We should remove "reverse" from the "state" property because we're using the "mode" property instead. Also, even though the visited reverse link is visually the same as default, I think we still need to create those variations. This way there is no confusion for a designer who's looking to set those specific properties in their local design file. If those properties are not there, they might think it's a DS error. |
@danbrady ahh.....good call about that reverse state! I neglected to clean that up after my first attempt at tackling this ticket. Should be all cleaned up now. And good point about the visited variation. I went ahead and added that too. Oh, and also changed the background to the primary blue...since, as you said before, that's the color mostly used instead of black. |
Description
There's a request to allow to be a white link instead of default blue #2602. We need to add this to Figma
Details
Tasks
Acceptance Criteria
The text was updated successfully, but these errors were encountered: