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

[Snackbar] Add a maxWidth property #12327

Closed
wants to merge 3 commits into from

Conversation

simoami
Copy link
Contributor

@simoami simoami commented Jul 29, 2018

  • Add a maxWidth parameter to specify the maximum width of the tooltip.
  • Default value is 300 to enhance readability of long text by default.
  • Specify maxWidth={0} to suppress the width constraint.
  • Add demo and api docs.
  • Add unit tests and pass linter validation.

@oliviertassinari oliviertassinari added new feature New feature or request component: snackbar This is the name of the generic UI component, not the React module! labels Jul 29, 2018
@oliviertassinari oliviertassinari changed the title Add an optional maxWidth parameter to the tooltip component. [Snackbar] Add a maxWidth property Jul 29, 2018
@mbrookes
Copy link
Member

@simoami Looks good!

Could you perhaps add spacing between the buttons following https://material-ui.com/demos/buttons/#text-buttons (and also for the CustomizedTooltips example)?

## Customized Tooltips

{{"demo": "pages/demos/tooltips/CustomizedTooltips.js"}}

Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for the change in order? Where we have customization examples they tend to come last.

Copy link
Member

@oliviertassinari oliviertassinari Jul 29, 2018

Choose a reason for hiding this comment

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

@mbrookes Trust is, I'm very hesitant to have this property. From my point of view, the main value of this pull request comes from the default value it's setting: max-width: 300.
Regarding having a dedicated property for that? Well, why not using CSS override?
I have the intuition that people will be more interested in the Customized Tooltips tooltip demo than in the max-width. Benchmarking the other tooltip implementations, I have seen very few having this type of customization. Anyway. I want to add analytics on the live documentation regarding demos usage. I think that we should sort the demos, when possible by their popularity, so overall, people scroll less in the pages to find what they are looking for.

What do you think?

Copy link
Contributor Author

@simoami simoami Jul 29, 2018

Choose a reason for hiding this comment

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

@oliviertassinari If it helps, I always scroll to the bottom and love viewing all the demos down to the last one :). Another way to know is to gauge the number of user questions that can be associated with not seeing the demos at the bottom. Do you ever have to say "Scroll down to the last demo example" and how often?

I also inserted the demo example before the last one on purpose, because it pertains to what is now a new standard property. It seemed odd to talk about css customization then document a basic property last. This is a shift from the pattern I'm used to see elsewhere.

Copy link
Member

@oliviertassinari oliviertassinari Jul 29, 2018

Choose a reason for hiding this comment

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

@simoami Oh, Interesting habit, I'm not the only one to start at the bottom then!

@mbrookes
Copy link
Member

I'm very hesitant to have this property. [...]
why not using CSS override?

That's a fair point. I appreciate the effort that @simoami has put in to this PR, but perhaps it should just be a customisation example instead.

@mbrookes
Copy link
Member

I think that we should sort the demos, when possible by their popularity, so overall, people scroll less in the pages to find what they are looking for.

Well, there's a certain order to them at the moment; simplistically: basic functionality, advanced functionality, customization – but within that there's certainly some room to rearrange things.

Perhaps the TOC for each page that we discussed previously is another way to reduce the need to scroll? Even a basic TOC would be better than none, although I do like your idea of a TOC that highlights the current section.

@simoami
Copy link
Contributor Author

simoami commented Jul 29, 2018

I'm very hesitant to have this property. [...]
why not using CSS override?

That's a fair point. I appreciate the effort that @simoami has put in to this PR, but perhaps it should just be a customisation example instead.

@oliviertassinari @mbrookes
I'm perfectly fine reducing this to a simple css prop. I think wrapping text by default is helpful if you happen to feed the tooltip with long text, as opposed to say having a one line 1000px wide tooltip. And It's harmless for normal usage. I also think the Popover component can benefit from this default behavior.

@mbrookes
Copy link
Member

A sensible default along with a customization example would be perfect. 👍

@oliviertassinari
Copy link
Member

Perhaps the TOC for each page that we discussed previously

@mbrookes Yes, having a TOC, if well executed, can be a significant improvement!

I also think the Popover component can benefit from this default behavior.

@simoami Same logic for the Popper component. I don't know. It's a bolder move than with the Tooltip.

A sensible default along with a customization example would be perfect. 👍

I love this plan :). One step at a time. If people complain more about this use case, we add a property.

@simoami
Copy link
Contributor Author

simoami commented Jul 29, 2018

@mbrookes @oliviertassinari
Ok, and just to double check, is everyone ok with 300px as a good default? Feel free to see the demo again.

One more question: do you want to see a new section for max-width customization or an extra demo block under "Customized Tooltips" ?

@mbrookes
Copy link
Member

mbrookes commented Jul 29, 2018

I don't have a strong opinion on the default, but given that tooltips are meant to be short, perhaps narrower than 300?

do you want to see a new section for max-width customization

It might be worthwhile to show the default alongside the customization, which would suggest its own section. @oliviertassinari, thoughts?

@oliviertassinari
Copy link
Member

As you guys see fit. I see some value in a 200px vs 300px min-width as well as two demos vs a single demo.

@simoami
Copy link
Contributor Author

simoami commented Jul 30, 2018

A default of 200px might be too restrictive. You still want to give regular sentences a chance to appear in a single line. Example:

300px vs 200px
screen shot 2018-07-30 at 1 45 09 pm

What do you think?

@simoami
Copy link
Contributor Author

simoami commented Jul 30, 2018

@mbrookes @oliviertassinari Take 2: #12338

@mbrookes
Copy link
Member

Closing in favor of #12338

@mbrookes mbrookes closed this Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: snackbar This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants