-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
Fixes #211083 - change position of terminal icon picker #216983
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
@bricefriha we don't want a new UI component just for the color picker, that would need a bunch of additional maintenance that is not worth it. For example, I can see several layout issues immediately and. The quick pick is fine for this, we only want to position the icon picker specially since it supports that. |
@Tyriar I get you. I'm sorry if I misunderstood anything. In my opinion, I would find it confusing to have the colour picker on the top while the icon one is alongside the terminal tab. |
It's not ideal, but there's too much overhead in creating another widget to support this niche use case. We switched to the icon picker purely because the work to get a polished widget happened as part of the user profiles work. |
Makes perfect sense Daniel! I'll roll back the changes on the colour picker ASAP and push again. Would that work for you? |
cc4a47e
to
2249f73
Compare
@Tyriar I just rolled back the colour picker, let me know if I should change anything else (feel free to let me know that annoys you to when I mention you in replies, as well) |
95e6a46
to
9f084e3
Compare
Sorry, I wanted to rebase and I created a mess |
Per #211083, the position of both the terminal colour picker and the icon picker had to be moved closer to the terminal tab.
I took over PR #212174, which was left without its requested changes being addressed. I would give all the credit to @albarv340 for their work on the first part.
Icon picker placement
Additional context
On the initial PR, the icon picker was only positioned correctly when there was more than one terminal tab (as shown in the screenshot on the right). However, the default position remained at the top of the window.
Colour pickerAdditional context
As I told @Tyriar on the issue, the colour picker used a `quickPicker, ' which was harder to modify without risking impacting other elements relying on the same class. So, I created it from scratch, similar to the icon picker. Let me know if you'd rather I use another method.