Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Terminal color picker tweaks #10219

Merged
11 commits merged into from Jun 10, 2021
Merged

Terminal color picker tweaks #10219

11 commits merged into from Jun 10, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 26, 2021 •
edited by DHowett
Loading

The flyout wasn't very polished, so I did some adjustments.
It's all visual changes, functionality should be the same.

  • made the flyout use OverlayCornerRadius and 16px padding (to match WinUI 2.6)
  • changed ColorPicker to muxc:ColorPicker for new styles (the color schemes picker too)
  • changed "Custom" Button into a ToggleButton
    • no longer needs ellipsis - localization files should be updated
  • OK button was moved to the right and uses accent color
  • adjusted margins and padding
  • tweaked the color boxes to look like the ones in color schemes

collapsednew expandednew

Validation Steps Performed

  • Color picker in settings UI still works ✔️
  • Color picker for tabs still works ✔️

@ghost
Copy link

ghost commented May 26, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@DHowett
Copy link
Member

DHowett commented May 28, 2021

(Thanks for this! We're a bit busy with the influx of new issues since we shipped 1.8/1.9, but we don't mean to ignore you. I'm shocked that we weren't using the right ColorPicker this whole time but WHATEVER

I hope we're using the right one in the settings editor 😁)

@ghost
Copy link
Author

ghost commented May 28, 2021

Thanks for the response!
The settings ColorPicker wasn't the mux one either. Half of this PR is only for changing that one 😄

lhecker
lhecker approved these changes Jun 6, 2021
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Review this PR with whitespace changes suppressed. ^^

I've built these changes locally and they're a real visual improvement. Nice! 👍
(I couldn't spot any behavior errors either.)

There are more changes I'd love to see in the future (ensuring the flyout doesn't touch window borders, reduce the margin above the "More" button, etc.), but they certainly don't have to be done in this PR.

@ghost
Copy link
Author

ghost commented Jun 6, 2021

Thanks for the review!
I made the flyout have an effective 8px margin. The "more" button is included in the default ColorSpectrum, so it can't be easily updated.

@lhecker lhecker added Area-Settings UI Anything specific to the SUI Area-User Interface Issues pertaining to the user interface of the Console or Terminal Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal. labels Jun 10, 2021
DHowett
DHowett approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks excellent. Thanks so much!

miniksa
miniksa approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful. I love it. Thank you!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 10, 2021
@ghost
Copy link

ghost commented Jun 10, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit d0d3cc6 into microsoft:main Jun 10, 2021
@ghost ghost deleted the terminal-color-picker-tweaks branch June 14, 2021 06:38
@ghost
Copy link

ghost commented Jul 14, 2021

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jul 14, 2021
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft Awaiting requested review from zadjii-msft

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees
No one assigned
Labels
Area-Settings UI Anything specific to the SUI Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DHowett @lhecker @miniksa