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

Added an option to insert a margin from the bottom of the screen #96

Closed
wants to merge 6 commits into from

Conversation

kryjex-av
Copy link

In addition:

  • iOS: Fixed a bug when the title of the snackbar is large and there is
    no action button (title will occupy the entire snackbar).
  • Android: Remove elevation.

@cooperka
Copy link
Owner

Hi @kryjex, thanks for the contribution.

The new bottom prop seems useful as a workaround for #15. Will you please also document the prop in the README?

Do you have any specific reason for removing the Android elevation? I prefer to match the Material Design guidelines as much as possible.

Can you give an example of what happens on iOS with the title bug? A screenshot would be helpful.

Finally, It looks like there are several distinct changes in this one PR; it's usually nice to split up any changes into separate PRs so they can be reviewed individually. Let's keep this PR just for the new margin, and we can fix the title bug separately. Thanks!

@kryjex
Copy link

kryjex commented Nov 26, 2018

Hi :)

Sure. I'll add the prop in the README file and an example button in a next commit.

Do you have any specific reason for removing the Android elevation? I prefer to match the Material Design guidelines as much as possible.

Yup. I did because when the bottom prop is used and elevation is not removed, iOS and Android seems visually inconsistent (Android has a shadow, iOS not). Maybe should be removed if bottom prop is defined? It did not make sense to me because when the bottom prop is not defined the elevation it is not appreciated. Another option is to create the elevation prop (true by default) and deactivate if the user needs it. Please, what do you think?

Can you give an example of what happens on iOS with the title bug? A screenshot would be helpful.

Sure, the problem is visible when the title of the snackbar is two lines long. There was always a space reserved for a button, even when it was not defined.

before

Now the snackbar title use the entire space if the action is not defined:

now

Finally, It looks like there are several distinct changes in this one PR; it's usually nice to split up any changes into separate PRs so they can be reviewed individually. Let's keep this PR just for the new margin, and we can fix the title bug separately. Thanks!

Sorry about that. So I remove changes related to snackbar title and action button on iOS and then create another commit and PR? Please, let me know and I'll do.

Thanks!

@cooperka
Copy link
Owner

So I remove changes related to snackbar title and action button on iOS and then create another commit and PR?

Yes please, thanks!

Maybe should be removed if bottom prop is defined?

I think the default elevation should be kept; generally the only reason to show the snackbar higher up is to put it above some sort of action bar, in which case the snackbar should still be elevated. In fact the Material Design guide specifically says "Avoid displaying a snackbar container without elevation" so I think we should respect that.

Can you also make sure the animation is changed if the bottom prop is included? Instead of zooming up from the bottom, it should zoom in from the center: video (or backup link).

@cooperka
Copy link
Owner

Closing due to inactivity. Happy to re-open later.

@cooperka cooperka closed this Feb 14, 2019
@holden-caulfield
Copy link

Hi @cooperka I'd really like this solution (the bottom property).

On my app I have a bottom tab bar and I should show the snackbar above that tab bar. In my case at least, the tab bar should still slide up (think about it as if the "bottom" of the screen is actually there and the tab bar should not count)

What would be needed for this PR to be merged? I'm happy to give it a shot and complete what may be missing

@cooperka
Copy link
Owner

Hi @holden-caulfield, if you'd like to tackle this please open a fresh PR so we can discuss separately. There's a lot going on in this one.

Displacing the tab bar is a tricky issue, see #11, but if you have a solution that would be fantastic (please submit a separate PR for this).

If you simply want to add a bottom offset, that should be simple, as long as you use the animation mentioned above in #96 (comment). Thanks for your interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants