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

Consistently show ellipsis in place of overflowing tree content #7162

Closed
wants to merge 1 commit into from
Closed

Consistently show ellipsis in place of overflowing tree content #7162

wants to merge 1 commit into from

Conversation

garethwhittaker
Copy link
Contributor

What it does

Fixes: #7145

The styling of overflowing tree content is currently inconsistent. An example of this is shown in the screenshot below.

An ellipsis should now be shown in place of overflowing content in all tree views.

How to test

Open a project and reduce the width of the tree view until content would extend beyond the available space. In all tree views an ellipsis should replace the overflowing content.

Include the Git Lens extension in this testing (#6921)

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the ui/ux issues related to user interface / user experience label Feb 16, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I tested the functionality and it looks great! 👍
I verified using the npm scripts and gitlens extensions.

Thank you for your contribution!

@akosyakov
Copy link
Member

@vince-fugnitto It looks like a breaking change, also grow class reads like it should support ellipses already. I'm afraid to merge it. Do you think it would not change css semantics for any extended trees?

@akosyakov akosyakov added the tree issues related to the tree (ex: tree widget) label Feb 17, 2020
@vince-fugnitto
Copy link
Member

@vince-fugnitto It looks like a breaking change, also grow class reads like it should support ellipses already. I'm afraid to merge it. Do you think it would not change css semantics for any extended trees?

Ahh I see what you mean. I think the grow class has problems displaying ellipsis for the tree-view due to the display property. Can we perhaps limit this change only to the tree-view so that not all trees are affected?

@vince-fugnitto
Copy link
Member

@garethwhittaker I think you can limit the change to trees contributed by plugins as to not break any extended trees contributed by internal extensions.

@garethwhittaker
Copy link
Contributor Author

👍 @vince-fugnitto let me know if this change looks suitable.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The change fixes the issue with the tree-views and does not affect any extended trees (from core). @akosyakov are you fine with this approach?

@akosyakov
Copy link
Member

Does it mean that there is no way to restyle it without introducing new classes? Why it does not break other trees?

@@ -38,3 +38,9 @@
align-items: center;
height: 100%;
}

.theia-TreeNodeSegmentEllipsis {
Copy link
Member

Choose a reason for hiding this comment

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

it should be .theia-tree-view .theia-TreeNodeSegmentEllipsis

@akosyakov
Copy link
Member

Is it still relevant after #7237. I believe the proper solution is similar to #7237 (comment)

@vince-fugnitto
Copy link
Member

Is it still relevant after #7237. I believe the proper solution is similar to #7237 (comment)

Thank you for the fix @garethwhittaker, however, it looks like the pull-request #7237 has already fixed the issue with a proper solution. We look forward to future contributions 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tree issues related to the tree (ex: tree widget) ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tree-view: update styling when the tree row content cannot be fully displayed
3 participants