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

[Toolbar] toolbar-title overflow: Set width in the example #3463

Merged
merged 1 commit into from
Feb 26, 2016

Conversation

tintin1343
Copy link
Contributor

This references issue: #3192

Previously submitted PR: #3250

  • Since the issue still persists even after the last PR was merged, I decided to change the fix a bit. All other css changes in toolbar-title.jsx remains the same except for the width attribute.
  • The earlier logic of having the width based on the number of characters within the component jsx code seemed dodgy. I decided to move the width attribute as a part of the inline style in the example code instead.
  • This makes more sense to me since the use of toolbar might change based on the use-case and so does the title width which is dependent on the number of elements being present in a toolbar implementation.
  • I have included a value of width:150px based on the implementation in the example page but I understand that it is still a magic constant and my suggestion would be to add a note in the docs mentioning the same like its done for the popover ghost touch issue , as it is more of an implementation issue.
  • I could add the note if needed. @alitaheri @newoga need you opinion about it.
  • As you both know I have started work on redesign of the Toolbar component. This can work as a temporary fix till then.

P.S: I created a new branch and proceeded the issue in a fresh manner hence the new branch.

@tintin1343
Copy link
Contributor Author

Screenshots of the implementation:
screen shot 2016-02-24 at 4 28 38 pm

screen shot 2016-02-24 at 4 28 53 pm

@oliviertassinari
Copy link
Member

The earlier logic of having the width based on the number of characters within the component jsx code seemed dodgy.

Giving users the power to fix it seems better 👍. Can we remove the magic width value now?

@tintin1343
Copy link
Contributor Author

@oliviertassinari : Which magic width are you referring to? I have removed it from toolbar-title.jsx which had a width attribute as per the previous fix.

The width value of 150px is in the docs example page, which is to show the user how to implement it in their use-cases.

@oliviertassinari
Copy link
Member

Which magic width are you referring to?

Sorry I was confused not seeing this line removed https://github.com/callemall/material-ui/pull/3250/files#diff-6ac27b5e67882578a46d38f690fd414bR20.

Could you rebase 👍?

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 25, 2016
@tintin1343 tintin1343 force-pushed the toolbarTitle branch 3 times, most recently from 76115b6 to 965e5a4 Compare February 25, 2016 20:27
@tintin1343
Copy link
Contributor Author

Could you rebase 👍?

@oliviertassinari : Done.

@mbrookes mbrookes added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 25, 2016
@@ -37,7 +37,7 @@ export default class ToolbarExamplesSimple extends React.Component {
</DropDownMenu>
</ToolbarGroup>
<ToolbarGroup float="right">
<ToolbarTitle text="Options" />
<ToolbarTitle text="Options" style={{width: 150}} />
Copy link
Member

Choose a reason for hiding this comment

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

90 would be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing it to 90

mbrookes added a commit that referenced this pull request Feb 26, 2016
[Toolbar] toolbar-title overflow:  Set width in the example
@mbrookes mbrookes merged commit 25dcaa4 into mui:master Feb 26, 2016
@zannager zannager added the component: Toolbar The React component. label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Toolbar The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants