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

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

Merged
merged 1 commit into from Dec 10, 2020
Merged

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

merged 1 commit into from Dec 10, 2020

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
@@ -526,7 +526,7 @@ namespace winrt::TerminalApp::implementation
Controls::MenuFlyoutItem closeTabMenuItem;
Controls::FontIcon closeSymbol;
closeSymbol.FontFamily(Media::FontFamily{ L"Segoe MDL2 Assets" });
closeSymbol.Glyph(L"\xE8BB");
closeSymbol.Glyph(L"\xEDAE");
Copy link

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

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

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

@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
@@ -526,7 +526,7 @@ namespace winrt::TerminalApp::implementation
Controls::MenuFlyoutItem closeTabMenuItem;
Controls::FontIcon closeSymbol;
closeSymbol.FontFamily(Media::FontFamily{ L"Segoe MDL2 Assets" });
closeSymbol.Glyph(L"\xE8BB");
closeSymbol.Glyph(L"\xEDAE");
Copy link

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!

src/cascadia/TerminalApp/Tab.cpp Outdated Show resolved Hide resolved
@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

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 <[email protected]>
(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 <[email protected]>
@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