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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bricefriha
Copy link

@bricefriha bricefriha commented Jun 24, 2024

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 picker

Additional 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.

@bricefriha
Copy link
Author

@microsoft-github-policy-service agree

@Tyriar
Copy link
Member

Tyriar commented Jun 24, 2024

@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.

@bricefriha
Copy link
Author

@Tyriar I get you.
I'm a bit confused because @Narendherraj and @abhijit-chikane mentioned that it would be preferable to move the colour picker as well.
#211083 (comment)

I'm sorry if I misunderstood anything.
I can roll back the changes on the colour picker and only keep the quickpicker.

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.

@Tyriar
Copy link
Member

Tyriar commented Jun 24, 2024

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.

@bricefriha
Copy link
Author

Makes perfect sense Daniel!

I'll roll back the changes on the colour picker ASAP and push again.

Would that work for you?

@bricefriha bricefriha changed the title Fixes #211083 - change position of color and icon pickers Fixes #211083 - change position of icon picker Jun 25, 2024
@bricefriha bricefriha changed the title Fixes #211083 - change position of icon picker Fixes #211083 - change position of terminal icon picker Jun 25, 2024
@bricefriha
Copy link
Author

@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)

@mjbvz mjbvz removed their request for review June 26, 2024 15:43
@bricefriha
Copy link
Author

Sorry, I wanted to rebase and I created a mess

@jrieken jrieken removed their request for review June 27, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants