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

Fix#122454: Truncate the long terminal title #122620

Merged
merged 15 commits into from Jul 9, 2021

Conversation

suema0331
Copy link
Contributor

This PR fixes #122454
image

@ghost
Copy link

ghost commented Apr 29, 2021

CLA assistant check
All CLA requirements met.

Copy link

@glaukiol1 glaukiol1 left a comment

Choose a reason for hiding this comment

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

This does work correctly, for me this is approved

@Tyriar
Copy link
Member

Tyriar commented May 3, 2021

@aeschli thoughts on the changes to iconLabels? Should we make this span the baseline behavior instead?

@aeschli
Copy link
Contributor

aeschli commented May 4, 2021

I don't like the changes to iconLabels very much:
I think its strange that iconLabels will create a span for just one of the text fragments and give it a class name label-text. And then there's a css rules in actionbar.css that happens to know about that css rule

I'd suggest to keep the changes local to the terminal title and not try to generalize this already in the iconLabels.

getSingleTabLabel creates a string and renderLabelWithIcons breaks it up again in a series of strings and icon spans.
Instead I would suggest to construct the label spans directly in updateLabel: use iconLabel.renderIcon to create the icon span create spans for label and descriptions and so on. That give you much better control over the dom nodes created and class names used.

I see that getSingleTabLabel is also used in the constructor but I wonder if the action label can really be used as it's more than just a label (label with the icon placeholders)

Copy link
Contributor

@aeschli aeschli left a comment

Choose a reason for hiding this comment

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

@suema0331
Copy link
Contributor Author

see #122620 (comment)

@aeschli
Thank you very much for your comment. 
I changed to construct the label spans directly in updateLabel using  iconLabel.renderIcon.

@Tyriar Tyriar added this to the June 2021 milestone Jun 2, 2021
@meganrogge meganrogge modified the milestones: June 2021, July 2021 Jun 29, 2021
Co-authored-by: Daniel Imms <daimms@microsoft.com>
@Tyriar
Copy link
Member

Tyriar commented Jul 13, 2021

This got reverted in #128483

@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long terminal title hides terminal buttons
7 participants