-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[PT Run] Updated themes (dark/light/highcontrast) #4119
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.
This is amazing 🔥.
@niels9001 |
#2988 I really wish there was a larger discussion on before the work was done |
Hey @niels9001 Can you please separate the border brushes of searchbox and resultlist box, so as to not impact #2988😊 |
@AnuthaDev That might be an issue, yes. Not sure where we are with that PR, so let's follow up there - I'm happy to make any changes if needed to solve a potential conflict I saw your comment about breaking ClearType due to (semi)-transparency. What specific brushes are impacted at the moment that you'd recommend to update? And is there any documentation I can check out, wasn't aware of this. |
@niels9001 Read the remarks section here: https://docs.microsoft.com/en-us/dotnet/api/system.windows.media.renderoptions.cleartypehint |
If you want to dive really deep: |
The PR #2988 is a good idea but it was work done without collaboration. Niels helped properly get our HC settings correct in this PR. |
@AnuthaDev The latest commit changed the transparency to fully opaque colors - let me know if this solves your concern wrt ClearType. Only the 'focus' color (AccentBrush) is still using opacity, we might want to update that as well but maybe in a new PR. |
@niels9001 Unfortunately its a bit trickier than just fixing colors, because we are using a dropshadoweffect, We need to apply this fix to get clear text. I think a new issue should be opened for cleartype after this PR is merged. |
Opened #4172 for cleartype |
@niels9001, we're going to merge this in, cool? |
Cool! |
(Can't have a borderbrush if you have no borders.. ) |
Not sure if I understand - what specific lines/controls? Borders for both SearchBox and ResultsList have a BorderThickness of 1 (line 57 and 78). They are set to transparent in the light/dark themes - white/black when in high contrast mode, following the W10 windowing appearance. Let me know! |
@niels9001 In MainWindow.xaml, I had to replace the Border containers by Grid to improve font rendering, so the border being set to 1 won't work as there would be no border. Anyways, I found the solution, I will rebase and fix the issue. @crutkas Good to go👍 |
Summary of the Pull Request
Merge together with #4165 so the correct colors/themes are applied.
PR Checklist