FORMS-18740: State of active component lacks 3 to 1 contrast ratio @sunnym @vavarshn #22
Conversation
|
Clones PR #18 |
|
|
||
| &--active { | ||
| color: $accent; | ||
| color: $black; |
There was a problem hiding this comment.
@stefan-dragomir It is ok to change to black from accent color of theme, if design team has recommended, as you mentioned in JIRA. Please check, are tabs(tabsontop and vertical) also facing same issue of contrast? They use same colors. Or you can also check with design team, if we should make both follow similar color scheme, as is the case now
There was a problem hiding this comment.
Please add the screenshots and mention the discussion which you had with the designer.
There was a problem hiding this comment.
Discussion archive link added in PR comment and Jira.
stefan-dragomir
changed the title
|
Approval still pending, new changes will come. |
src/site/_mixin.scss
Outdated
| display: flex; | ||
| align-items: center; | ||
| flex-flow: wrap; | ||
| align-items: end !important; |
There was a problem hiding this comment.
@stefan-dragomir Could you please show in screenshots, what change has it resulted in ?
There was a problem hiding this comment.
@stefan-dragomir since we are changing default-tab mixin, please add screenshots as to how vertical and horizontal tabs look, in desktop as well as mobile device. Also please add screenshots for wizard mobile resolution too.
cc: @vaibhav2601
There was a problem hiding this comment.
All screenshots added in ticket descriptions and Jira.
|
Discussion thread: Discussion printscreen: |
src/site/_mixin.scss
Outdated
| display: flex; | ||
| align-items: center; | ||
| flex-flow: wrap; | ||
| align-items: end !important; |
There was a problem hiding this comment.
@stefan-dragomir All changes seem fine in the PR. For this one, could you please elaborate why did we need to change flex-flow and alignment? Are we trying to align elements to end ?
There was a problem hiding this comment.
Yes, I needed to align to end instead of center because I added an ::after element as an underline right under the tab labels. This addition was pushing the label up, so aligning everything to bottom would keep the label in the same spot as before.
src/site/_mixin.scss
Outdated
| display: flex; | ||
| align-items: center; | ||
| flex-flow: wrap; | ||
| align-items: end !important; |
There was a problem hiding this comment.
@stefan-dragomir We can change end to flex-end. Works same here, but it is the correct value for vertical alignment.
also, for pseudo element, border-bottom instead of border-top will help prevent confusion.
Although the pseudo element approach is also correct for showing active tab, we could also have used border-bottom on tab itself to avoid having to add an extra pseudo element and its styling overhead. For now, you can choose to keep pseudo element.
There was a problem hiding this comment.
Align and Border updated.
The pseudoelement was added while the shape and the background of the tab were still visible, and a colored border bottom would have had rounded corners as well. I'll keep it as it is since it already matches design.
There was a problem hiding this comment.
Thanks @stefan-dragomir Background, Round borders can be managed by setting border bottom left and right radius values to zero.
But approving the PR for now as this approach is also not wrong, and alignments can be managed with applied CSS.
@vaibhav2601
Description
Related Issue
https://jira.corp.adobe.com/browse/FORMS-18740
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Before
Wizard Desktop and Mobile
Horizontal Tabs Desktop and Mobile
Vertical Tabs Desktop and Mobile
After
Wizard Desktop and Mobile
Horizontal Tabs Desktop and Mobile
Vertical Tabs Desktop and Mobile
Types of changes
Checklist: