Closed (fixed)
Project:
Claro
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
22 Aug 2019 at 11:10 UTC
Updated:
12 Sep 2019 at 11:09 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
ant1Comment #3
ant1Comment #4
ckrinaThanks for working on this @AntoineH! I've checked the design implementation and looks good. I'll leave the code review to @lauriii. :)
Comment #5
anevins commentedWow that looks very cool! Should we darken the green so that it has better contrast? The current green (I think) is #28aC67, which falls slightly short of a 3:1 contrast ratio between the green and the background white: https://webaim.org/resources/contrastchecker/
Comment #6
lauriii@anevins could you open separate issue to discuss the contrast ration of the focus ring? This color is used in various different places and it would be great to fix them all at once.
Comment #7
ckrinaFYI per the designs the green should be #26A769, with a 3.08:1 contrast that afaik should be enough for graphic elements. But it makes sense to address it in another issue together with other components.
Comment #8
lauriiiThis looks great!
I found a pre-existing issue that becomes worse. According to the design system, there should be 8px between the form element and the label. However, our current implementation only has 6px, and when we add the focus effect, it renders very close to the label as a result. We should probably fix that as part of this issue.
Comment #9
fhaeberleI updated the form elements (checkbox&radio) with the new spacing. Please review! :)
Comment #10
fhaeberleFixing whitespace lint error.
Comment #11
quironJust my 2 cents, isn't
outline(https://www.w3.org/TR/css-ui-4/#outline-props) the right CSS property to managing this kind of status highlighting?Comment #12
lauriii@quiron
outlinesupports only some very basic use cases. Our design has multiple outlines (2px white, 3px green) which isn't supported by outline. Because of Windows high contrast doesn't render box-shadow, we define a transparent outline that will be visible there.It seems like the latest patch doesn't work as expected. With the latest patch there's no spacing between the input and the label at all.
Comment #13
fhaeberleSorry, changed the variable name but forgot to update!
This patch should be working.
Comment #15
lauriiiThank you! Looks great! Committed and pushed.