-
Notifications
You must be signed in to change notification settings - Fork 0
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: larger names shrunk in variable tab #5
Conversation
@misiekhardcore Assigning you for review. Please have a look. 👀 |
Can you tell me how to tests that? I have no idea how to run this locally so I can see if it works |
That is our general practice, it works in well maintained projects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So few questions/observations
- When you changed the display from
flex
toinline-block
, the alignment of the elements broke (it is not centered anymore) also themax-width
caused the element does not have the full with of the list
- when the ellipsis kicks in, you are not able anymore to see the full name in any way. I would suggest adding a
title
prop so when user hovers over the list item, the full label pops up - Also kinda related question, shouldn't also some kind of overflow prevention be done to the diagram elements themselves? Cause now when you give an element a long name, it overflows the eleemnt
- maybe a better approach to ellipsis is just preventing the svg element with
flex-shrink: 0
?
@nikku I won't argue with that, I just know different approaches, like |
In our sphere:
You can try |
If this repository does not live up to our standards, then we want to lift it up 🙂 |
9212c53
to
9d5d4fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better already cause the element takes the whole space as it should and the ellipsis is there, but the alignment issue remains. You set display: inline-block
on the element and at the same time the align-items: center
which works only with flex box so it does not work and there is no horizontal alignment between svg and text
I think we need to wrap the text in its separate element to give it the ellipsis so the whole .cds--tree-node__label
box can stay flex
.
Regarding flex-shrink: 0
I am not sure why setting it on .cds--tree-node__label svg
doesn't work for you.
Also, maybe worth looking into as well cause it is related, maybe not, text overflowing issue occurs for process name
And lastly, if you want to have a look together or talk about it feel free to ping me
9d5d4fb
to
4a69e33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This starts to look really good! Just two minor suggestions and we are good
4a69e33
to
35adbfa
Compare
Related to: camunda-modeler #4505
35adbfa
to
c9bbf3b
Compare
Related to: camunda/camunda-modeler#4505
Proposed Changes
Truncate variable names exceeding max width with ellipses for improved readability
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}