Merged
Conversation
carlos-zamora
approved these changes
Feb 2, 2022
Member
carlos-zamora
left a comment
carlos-zamora
left a comment
There was a problem hiding this comment.
Can we also take this time to replace all of the radio buttons with combo boxes in SUI? I think it'll just look and feel better. And it fixes some a11y issues for us off the bat.
Member
|
i do not believe we should make that change (combo boxes) this late in the cycle. It may have a localization impact depending on how we do it, and we have precious little time :)
|
This was referenced Feb 2, 2022
Member
|
Titlebar comparison (old top, new bottom) god bless: But then, this happens: I don't think the
|
Member
|
This one occurred when closing the window while the SUI was open. Repro'd 2/5 times I tried (the first two, and now I can't get it again...). There aren't really any Nav View changes here... are there? Crash 1I can't repro this again, so feel free to ignore.I suspect that this was the same build thing that caused the other crash. Note that there's no source linking for |
Member
EDIT: this was a local build error. NO idea what happened here. I do not love that this was a possible state to get into. Please ignore me
|
Contributor
Author
Did you expand the launch size expander? |
Member
right, inline means for the number box, not the Expander |
Member
ah yep that makes sense. Derp. |
DHowett
reviewed
Feb 2, 2022
src/cascadia/TerminalApp/App.xaml
Outdated
| <StaticResource x:Key="TabViewBackground" | ||
| ResourceKey="ApplicationPageBackgroundThemeBrush" /> | ||
|
|
||
| <SolidColorBrush x:Key="UnfocusedBorderBrush" |
Member
There was a problem hiding this comment.
can you change both of the UnfocusedBorderBrush resources as well? they should match the title bar. you can validate them by splitting a pane
Contributor
Author
There was a problem hiding this comment.
So it works for closing a pane, but for opening a pane the background is actually transparent (this issue exists in main)
Member
|
@msftbot make sure @zadjii-msft signs off |
ghost
added
the
AutoMerge
|
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
Member
|
we may have to ship with that wonky dropdown. can someone go through the entire settings page on (1) super narrow mode and (2) super wide mode? |
Contributor
Author
The wonky dropdown has been fixed!
I did and everything looked fine (except the actions/color schemes pages which we already know need follow up), but couldn't hurt to have another pair of eyes on it I guess |
lhecker
approved these changes
Feb 2, 2022
Member
|
I haven't checked this PR out locally, but I can do that in about an hour. 🙂 |
zadjii-msft
approved these changes
Feb 2, 2022
ghost
merged commit
DHowett
pushed a commit
that referenced
this pull request
Feb 2, 2022
- The add new profile page now uses a dropdown rather than radio buttons - Subheaders, breadcrumb bar, buttons etc are now all centralized when the window is maximized (so they all align with the expanders now) - We no longer override the titlebar colors and instead use the xaml defaults (these still aren't great but at least we will get the fix automatically when it happens upstream) - Breadcrumb bar no longer has a negative margin, so there's no weird overlap that happens when the window becomes small - The number boxes for launch size and font size now use the `Inline` placement mode rather than compact, allowing modification to the number with fewer clicks - Textboxes now have a greater max width so they can occupy more space in the expander if needed (cherry picked from commit 0a545b7)
This was referenced Feb 7, 2022
Closed
ghost
pushed a commit
that referenced
this pull request
Feb 15, 2022
This has been a saga. Basically, any resources in `App.xaml` aren't going to be able to reference other theme-aware resources. We can't change the theme of the app at runtime, only elements within the app. So we can't use `ApplicationPageBackgroundThemeBrush` in app.xaml, because it will ALWAYS be evaluated as the OS theme version of that brush. * regressed in #12326 * See also #10864 * #3917 CANNOT be fixed in the same way. We're lucky here that the TabView uses a `{ThemeResource TabViewBackground}` in markup to set the bg. We're not similarly lucky with the Pane one. * [x] closes #12356 * [x] Tested manually. You can confirm, my eyes are bleeding from the OS-wide light mode
DHowett
pushed a commit
that referenced
this pull request
Feb 16, 2022
This has been a saga. Basically, any resources in `App.xaml` aren't going to be able to reference other theme-aware resources. We can't change the theme of the app at runtime, only elements within the app. So we can't use `ApplicationPageBackgroundThemeBrush` in app.xaml, because it will ALWAYS be evaluated as the OS theme version of that brush. * regressed in #12326 * See also #10864 * #3917 CANNOT be fixed in the same way. We're lucky here that the TabView uses a `{ThemeResource TabViewBackground}` in markup to set the bg. We're not similarly lucky with the Pane one. * [x] closes #12356 * [x] Tested manually. You can confirm, my eyes are bleeding from the OS-wide light mode (cherry picked from commit 5ba0d61)
zadjii-msft
pushed a commit
that referenced
this pull request
Mar 3, 2022
- The add new profile page now uses a dropdown rather than radio buttons - Subheaders, breadcrumb bar, buttons etc are now all centralized when the window is maximized (so they all align with the expanders now) - We no longer override the titlebar colors and instead use the xaml defaults (these still aren't great but at least we will get the fix automatically when it happens upstream) - Breadcrumb bar no longer has a negative margin, so there's no weird overlap that happens when the window becomes small - The number boxes for launch size and font size now use the `Inline` placement mode rather than compact, allowing modification to the number with fewer clicks - Textboxes now have a greater max width so they can occupy more space in the expander if needed
zadjii-msft
added a commit
that referenced
this pull request
Mar 3, 2022
This has been a saga. Basically, any resources in `App.xaml` aren't going to be able to reference other theme-aware resources. We can't change the theme of the app at runtime, only elements within the app. So we can't use `ApplicationPageBackgroundThemeBrush` in app.xaml, because it will ALWAYS be evaluated as the OS theme version of that brush. * regressed in #12326 * See also #10864 * #3917 CANNOT be fixed in the same way. We're lucky here that the TabView uses a `{ThemeResource TabViewBackground}` in markup to set the bg. We're not similarly lucky with the Pane one. * [x] closes #12356 * [x] Tested manually. You can confirm, my eyes are bleeding from the OS-wide light mode
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Inlineplacement mode rather than compact, allowing modification to the number with fewer clicks