-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Semantic slot updates nov2018 #7176
Semantic slot updates nov2018 #7176
Conversation
@betrue-final-final - I believe you're familiar with these semantic slot updates, can you please review the screener test for this PR? |
It appears that the disabled icon slot is wrong. The disabled check, toggle, should be #a6a6a6 and not #c8c8c8, and the toggle thumb looks like it got lighter in code, but the change was subtle. |
@bennettclark It looks like there are some regressions in screener - is there a plan to fix these? |
@aneeshack4 @betrue-final-final I'm waiting to hear back from Sirena to confirm that the change of disabledBodyText from #c8c8c8 to #a6a6a6 wasn't a mistake or typo, if she confirms that was the value she was given then I'll be starting a convo with Ben and Sirena to get that sorted out |
The text change was correct but the icons and actual check box need to remain #c8c8c8. This was something that Sirena, myself and Janet worked out before. |
@betrue-final-final - can you take a look at the screener results now? It looks like the changes are all on the text color now, which I believe you said was correct. (Also, I'm not sure what the change is on the toggle result.) |
@srideshpande @dzearing @aneeshack4 - Ben has signed off on these design changes and all checks have passed, so as soon as you have a chance to take a look and sign off on this PR that would be much appreciated! |
LGTM @srideshpande @dzearing do y'all have anything to add? I think we need 1 code owner review. |
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.
Approving for the choice group changes. |
@bennettclark looks like there's a merge conflict in Toggle.styles.ts |
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.
approving for DatePicker, it's just a snapshot change there
* hand cherry picked updates from PR #7176 * change logs * change single quotes back to double quotes * add back deprecated slot for backwards compatibility * newlines at end of files * update disabledBodySubtext for non-text elements that used disabledBodyText
🎉 Handy Links: |
🎉 Handy Links: |
🎉 Handy Links: |
🎉 Handy Links: |
🎉 Handy Links: |
Pull request checklist
$ npm run change
Description of changes
Updated semantic slot list and variant logic with new slots and values from designers. The new snapshots are from the color value updates. Also, api files needed to be updated for the changed packages.
It's a lot of files to look through, if you're looking for the files that were modified to make all these new snapshots, you'll want to look at:
ISemanticColors.ts
ISemanticTextColors.ts
theme.ts
_semanticSlots.scss
variants.ts
Also various components were updated to change disabledBodyText to the new disabledBodySubtext slot (disabledBodyText is only for text color, while disabledBodySubtext is used for other disabled elements like borders), per Ben and Sirena's direction.
Microsoft Reviewers: Open in CodeFlow