-
Notifications
You must be signed in to change notification settings - Fork 759
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
[Style] Update modals to increase readability on High-Contrast Theme #1402
Conversation
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 PR!
Left some comments for some minor improvements. Otherwise, great work!
@@ -218,8 +218,12 @@ html { | |||
} | |||
|
|||
.dialog { | |||
--dialog-bg: var(--neutral-1); | |||
--dialog-bg: var(--neutral-16); | |||
color: var(--neutral-4) !important; |
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.
Try a similar approach to what I wrote below for the --error-text
variable. It would be great to have a --dialog-color
variable or something similar that each theme could control without having to use `!important.
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.
We've been trying this approach with no luck yet, but we'll keep looking into it!
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've added a commit that's working on my end. Please try it locally and see if that works! 😃
Also, it would be great if you could add an entry to the change log. Under Unreleased and above v4.3.3 you can add a new section for Fixed and put your entry under that. |
Pull Request Test Coverage Report for Build 2255
💛 - Coveralls |
Over in the BotBuilder SDK, we have a script that pulls the PR comments for all PRs in a given date range. We use that to create the release notes & changelog at the end of each cycle. You guys are more than welcome to share that! |
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.
Hold until design review is performed.
@justinwilaby What's the process for a design review? |
@cleemullins it's as simple as assigning a Bot Framework Designer! I'll go ahead and do this. @mewa1024 can you revisit the direction to override the theme variables in all modals? I'm wondering if this break accessibility requirements in High Contrast mode. Justin tells me the VSCode does something similar. This is odd to me. |
Yes, the black background for High Contrast mode is fine. |
Fixes issue #1312
Changes made
.dialog
color and bg-color of the high-contrast theme to match the theme's palette.