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 support for multiline messages #4294

Closed
wants to merge 0 commits into from

Conversation

andrejunges
Copy link
Contributor

@andrejunges andrejunges commented May 18, 2016

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@mbrookes @oliviertassinari
Second attempt to make the Snackbar support multiline text. 😄
I had some problems in that other branch and ended up creating it from the master.
A few notes:

  • Based on our previous conversation I could check the element height to define if it's multiline and I keep that thought. However since our SnackbarBody is a functional component, I can't handle the ref there - so I pass the ref callback and the element itself from the parent Snackbar. I know its not that pretty and I'm open to suggestions.
  • This implementation doesn't support that mobile multiline with button version. Last example here. Do you think its necessary?
  • I added the messageProp in the propTypes because the lint was complaining. Is that the correct way (using the @ignore)?

Closes #3860

@mbrookes mbrookes added new feature New feature or request PR: Needs Review labels May 18, 2016
@mbrookes
Copy link
Member

@andrejunges Thanks for resubmitting. 👍

The mobile multi-line would be nice to have for the sake of completeness, but I don't think it's necessary for this PR. If someone really needs it, they can contribute separately.

I'll let @oliviertassinari or one of the others comment on the approach.

@mbrookes
Copy link
Member

@andrejunges I submitted a PR to your fork that simplifies the example to use a single Snackbar. The current example allows two Snackbars to open at the same time.

However, the new example code highlights a different issue. When the Snackbar opens, it takes the height of the previously open Snackbar, so a multiline that follows a single line is single line height.

There also seems to be an animation glitch when multi-line exits. It stops after animating down what appears to be single-line height, before disappearing. (That was there before tihs change.)

@mbrookes mbrookes 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 May 20, 2016
@nathanmarks
Copy link
Member

@andrejunges I'm not sure that the complexity of this implementation is worth it for the dynamic margin. There's also some other things here that should be unnecessary with flexbox, for eg, setting line-height at the root to 80px.

@andrejunges
Copy link
Contributor Author

@mbrookes Im out of good ideas at the moment to fix this issue - with this approach we have to define the multiline based on the element's height we would need to rerender twice if the user changes the open state and the message in the same time.. so the second time it could get the new height, but I dont like that..

However, the new example code highlights a different issue. When the Snackbar opens, it takes the
height of the previously open Snackbar, so a multiline that follows a single line is single line height.

Regarding the animation issue I'd say all we need to do is check if its multiline in the Snackbar getStyles function and pass the right height in the transform:
translate3d(0, ${isMultiline ? 80 : desktopSubheaderHeight}px, 0),

@nathanmarks I can remove the line-height- I just let it there because I could be missing something.. I think that this way is much easier than setting the margins manually... do you think that doing it manually would be better?

@@ -26,20 +26,25 @@ function getStyles(props, context) {
} = context;

const isSmall = width === SMALL;
const isMultiline = props.messageElement && props.messageElement.offsetHeight > 20;
const rootHeight = isMultiline ? '80px' : `${desktopSubheaderHeight}px`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoded to 80px? Won't this just make it support 2 lines and not arbitrarily many lines?

@mbrookes
Copy link
Member

mbrookes commented Jun 5, 2016

Hardcoded to 80px? Won't this just make it support 2 lines and not arbitrarily many lines?

Correct:

image

@oliviertassinari
Copy link
Member

@andrejunges Thanks for this second attempt to support multilines Snackbar.
My biggest concern with this PR is the hardcoded height. You raised a good point with the multi height example.
How could we solve it?

I think that it would be better to use a flexblox approach to get to correct layout.
However, we need to know the height of the Snackbar to apply the right transition. We could solve this issue by measuring the actual element height before applying the transition.

@mbrookes
Copy link
Member

My biggest concern with this PR is the hardcoded height.

Why is that a concern?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 19, 2016

Why is that a concern?

Basically, no matter the solution we are going to choose, we need to measure the layout height.
Hence, I would rather choose the simpler approach.
Handling 3 differents height cases sounds more complex than relying on the flex layout algorithm.

@andrejunges
Copy link
Contributor Author

so, you would like it to work based on margins / flex and a max-height value to limit it in two lines? @oliviertassinari

@oliviertassinari
Copy link
Member

@andrejunges Yes, we could rely on the margin / line-height to make sure the two lines cases is 80px tall. I have just realised another advantage of this approach. That makes style customizability simpler.
E.g. someone might want to increase the font-size.

@andrejunges andrejunges closed this Jul 7, 2016
@muibot muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 7, 2016
@nehalbhanushali
Copy link

Just a question.
Material design spec says multiline should be 80. Does it not mean it should be that much or are we flexible with that?

@oliviertassinari
Copy link
Member

@nehalbhanushali The height isn't supposed to be fixed. It's depending on the content that we render.

@mbrookes
Copy link
Member

@oliviertassinari Are you sure about that? #4294 (comment)

@oliviertassinari
Copy link
Member

@mbrookes What about the 118px height example?

@mbrookes
Copy link
Member

@oliviertassinari Alright clever-clogs. :) The point being, there are specific fixed heights defined for single and multi-line, rather than being variable based on content. Also, at some point we'd agreed that for relative simplicity, the 118px version wasn't strictly necessary for a multiline PR.

@jasan-s
Copy link

jasan-s commented Oct 26, 2016

on smaller screen i.e iphone 5 the text limit is very short before the action button stops showing. Any update on fix merge to master

@SpencerCDixon
Copy link

Why was this closed? Does anyone know of a workaround to be able to display large messages by chance?

@oliviertassinari
Copy link
Member

@andrejunges This was close as we migrated our effort toward the next branch. Here is the most up to date thread #6356. We are close from finishing the migrating, feel free to give us a hand 👋 😄 .

@SpencerCDixon
Copy link

SpencerCDixon commented May 18, 2017

Ahhh, great! Thanks, just wanted to make sure this didn't get lost in ether.

For folks who need bigger snackbar messages before next is ready this was the fix for me:

attach this inline style to bodyStyle prop

const bodyStyle = {
  lineHeight: 'auto',
  padding: '20px',
  height: 'auto',
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants