[Toolbar] toolbar-title overflow #3250
Conversation
newoga
changed the title
| toolbar, | ||
| } = state.muiTheme; | ||
|
|
||
| return { |
There was a problem hiding this comment.
Could we keep this existing block?
There was a problem hiding this comment.
I think this PR accidentally undoes the migration for this component 😁
@tintin1343 Could you see if you could reapply your changes to the latest version in master?
There was a problem hiding this comment.
It was my bad. I did some extra commits when I should have not. I am making the changes in the latest version of master as we are speaking.
I will resubmit the changes.
There was a problem hiding this comment.
No problem, thanks @tintin1343!
oliviertassinari
added
PR: needs revision
tintin1343
force-pushed
the
toolbar-title
branch
from
94ff07a
to
6883fee
Compare
|
I made the suggested changes. @newoga @oliviertassinari |
| textOverflow: 'ellipsis', | ||
| whiteSpace: 'nowrap', | ||
| overflow: 'hidden', | ||
| width: (props.text.length > 40) ? 150 : 'inherit', |
There was a problem hiding this comment.
Do we really need to set the width here?
There was a problem hiding this comment.
Yes we do for the long text to be converted to ellipses. Since the parent div has a width of 100%. I tried it without the width but it doesn't work.
The width component actually depends on the use-case though. I have put a generic width statement which is at least a bit scalable.
Please find the explanation on the issue page: #3192
There was a problem hiding this comment.
@oliviertassinari : Are there any doubts w.r.t to the width attribute?
There was a problem hiding this comment.
I'm asking this because the AppBar component is not provinding any width. 40 and 150 look like magic numbers.
What if we where using flexbox here?
There was a problem hiding this comment.
Yeah. I agree 40 and 50 are indeed magic numbers. I tried using flex it didn't help.
Thats because the ToolbarTitle becomes a part of the ToolbarGroup and depends on the number of components being included in the ToolBar. 'AppBar' has title as a property where as ToolbarTitle is a component in itself and its width will depend on the width of its parent element.
Correct me if I am wrong here.
tintin1343
force-pushed
the
toolbar-title
branch
from
6883fee
to
5e16672
Compare
|
@oliviertassinari @tintin1343 I'm good with this for now. I think the |
|
@newoga : I agree. I have been working on fixing it today and realised that the component as whole needs a redesign. @oliviertassinari : If you feel weird about the magic constants I can just rewrite it with a % value maybe which can be changed by the user based on a usecase. |
tintin1343
force-pushed
the
toolbar-title
branch
from
5e16672
to
c61a9f5
Compare
|
@oliviertassinari @newoga : I wanted to know as to why am I getting one check as failed. Not able to figure it out. I have made the changes after my conversation with @oliviertassinari and him agreeing to keep the width attribute for now. Let me know in case you need anything else. Thanks ! |
|
We have a flacky test. This one is failing from time to time. It looks like a uniform distribution of p=0.1. We should fix this test at some point. |
tintin1343
force-pushed
the
toolbar-title
branch
from
c61a9f5
to
d75a674
Compare
|
@oliviertassinari : I ran the travis task again. All checks passed this time! |
oliviertassinari
added
PR: Review Accepted
and removed
PR: needs revision
|
That's not perfect but this change shouldn't harm anybody. |
[Toolbar] fix toolbar-title overflow
|
@oliviertassinari : I agree with you completely. But that behavior still exists in the original design even when the text is small and when you minimize the screen. I can start working on redesigning the component. Your welcome. |
Proposed Solution
The highlighted text in the below screenshot are the needed changes. The changes are to be included in the toolbar-title.jsx file:
This leads to the following change:
a) When its a long string :
b) when its a small string:
