Skip to content

Fix#122454: Truncate the long terminal title#122620

Merged
meganrogge merged 15 commits into
microsoft:mainfrom
suema0331:fix/122454
Jul 9, 2021
Merged

Fix#122454: Truncate the long terminal title#122620
meganrogge merged 15 commits into
microsoft:mainfrom
suema0331:fix/122454

Conversation

@suema0331

Copy link
Copy Markdown
Contributor

This PR fixes #122454
image

@ghost

ghost commented Apr 29, 2021

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@glaukiol1 glaukiol1 left a comment

Copy link
Copy Markdown

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

Tyriar commented May 3, 2021

Copy link
Copy Markdown
Contributor

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

@aeschli

aeschli commented May 4, 2021

Copy link
Copy Markdown
Contributor

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)

@aeschli aeschli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@suema0331

Copy link
Copy Markdown
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
Comment thread src/vs/workbench/browser/parts/media/compositepart.css
@Tyriar

Tyriar commented Jul 13, 2021

Copy link
Copy Markdown
Contributor

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