Skip to content

Change tab close and rename icons to better fit the UI#8424

Merged
DHowett merged 1 commit intomainfrom
unknown repository
Dec 10, 2020
Merged

Change tab close and rename icons to better fit the UI#8424
DHowett merged 1 commit intomainfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Nov 27, 2020

Summary of the Pull Request

Made the tab icon smaller so that it is the same size as the other icons

References

PR Checklist

  • Closes Tab close icon is too big #8419
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Nov 27, 2020
View reviewed changes
src/cascadia/TerminalApp/Tab.cpp Outdated
Controls::FontIcon closeSymbol;
closeSymbol.FontFamily(Media::FontFamily{ L"Segoe MDL2 Assets" });
closeSymbol.Glyph(L"\xE8BB");
closeSymbol.Glyph(L"\xEDAE");
Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo Nov 27, 2020

Choose a reason for hiding this comment

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

Segoe MDL2 icons mentions "EDAE ErrorBadge12". Using an error badge to represent closing a tab seems dubious to me, even if the glyph is currently similar.

Copy link
Author

@ghost ghost Nov 28, 2020

Choose a reason for hiding this comment

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

Then maybe I will try make the original icon smaller > <

Copy link

@eoussama eoussama Nov 30, 2020

Choose a reason for hiding this comment

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

Either that or use “F13D StatusCircleErrorX”

@ghost
Copy link
Author

ghost commented Nov 29, 2020 •
edited by ghost
Loading

I can't change the size using FontSize().

@zadjii-msft
Copy link
Member

zadjii-msft commented Nov 30, 2020

@Hegunumo Could you elaborate a bit more? Without actually checking if this worked, the following does seem to at least compile for me...
image

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 30, 2020
@ghost
Copy link
Author

ghost commented Nov 30, 2020

Yes it compiles, but no matter what integer I put to parentheses size of glyph won't change at all. I can't make it either smaller or bigger.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 30, 2020
eoussama
eoussama reviewed Nov 30, 2020
View reviewed changes
Copy link

@eoussama eoussama left a comment

Choose a reason for hiding this comment

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

F13D (StatusCircleErrorX)is a much smaller variant of the icon. It could work.

src/cascadia/TerminalApp/Tab.cpp Outdated
Controls::FontIcon closeSymbol;
closeSymbol.FontFamily(Media::FontFamily{ L"Segoe MDL2 Assets" });
closeSymbol.Glyph(L"\xE8BB");
closeSymbol.Glyph(L"\xEDAE");
Copy link

@eoussama eoussama Nov 30, 2020

Choose a reason for hiding this comment

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

Either that or use “F13D StatusCircleErrorX”

zadjii-msft
zadjii-msft approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

before after
image image

Definitely looks better to me, thanks!

@zadjii-msft zadjii-msft added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Needs-Second It's a PR that needs another sign-off labels Dec 1, 2020
@carlos-zamora
Copy link
Member

carlos-zamora commented Dec 3, 2020

I'm ok with this. Just fix the merge conflicts and I can approve/merge it. 😊

@DHowett
Copy link
Member

DHowett commented Dec 3, 2020

@carlos-zamora you can definitely approve something that has merge conflicts -- it cannot be merged even with your approval if there are conflicts.

@DHowett DHowett changed the title Changed to a smaller icon Change tab close and rename icons to better fit the UI Dec 10, 2020
@DHowett DHowett merged commit 6952f1a into microsoft:main Dec 10, 2020
@DHowett
Copy link
Member

DHowett commented Dec 10, 2020

thank you!

DHowett pushed a commit that referenced this pull request Jan 25, 2021
@kaihugo @DHowett
Change tab close and rename icons to better fit the UI (#8424)
ceb0551 
Closes #8419

Co-authored-by: KiminoriKaburagi <heipo_nogu@outlook.jp>
(cherry picked from commit 6952f1a)
@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

@ghost ghost mentioned this pull request Jan 28, 2021
Closed
@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
@kaihugo @mpela81
Change tab close and rename icons to better fit the UI (microsoft#8424)
3720adf 
Closes microsoft#8419

Co-authored-by: KiminoriKaburagi <heipo_nogu@outlook.jp>
@zadjii-msft zadjii-msft mentioned this pull request Mar 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miniksa miniksa Awaiting requested review from miniksa

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees

No one assigned

Labels

Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tab close icon is too big

5 participants

@zadjii-msft @carlos-zamora @DHowett @eoussama @KalleOlaviNiemitalo