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

Support for SnackBar style notifications #39

Closed
wants to merge 9 commits into from
Closed

Conversation

gilescm
Copy link
Contributor

@gilescm gilescm commented May 23, 2020

Hi, I've recently used this package to show a "No internet connection" message, and the use case was that the notification appear at the bottom of the screen like the SnackBar widget.

I saw that there's no customisability in this regard. So I've made a few very small changes to showSimpleNotification , showOverlayNotification and added a new BottomSlideNotification to allow for behaviour that mimics SnackBar, with all the other benefits of this package.

Improvements to my implementation can be made, for instance BottomSlideNotification only has a small difference to TopSlideNotification so they could be merged with an optional position parameter passed in. However, I didn't want to change the code too much so didn't implement this.

Let me know what you think and if this is something you would want to bring into the main branch.

@boyan01 boyan01 changed the base branch from dev to master May 23, 2020 14:30
@boyan01 boyan01 changed the base branch from master to dev May 23, 2020 14:30
@boyan01
Copy link
Owner

boyan01 commented May 23, 2020

Hi , Thanks for your contribution. The notification pops up from the bottom, which is a good idea.

I noticed that these commits are merged into the dev branch, it may be far away for merge this branch #29 into the master branch...

So I think this changes can be merged directly into the master branch, but there are two points that may need reconfirm:

  1. Is it a bit inappropriate to name isSnackBar directly in the API? After all, the behavior of SlideBottomNitification and SnackBar are different.
  2. Is the notification that pops up from the bottom compatible with the bottom navigation bar of Android / iOS?

@gilescm
Copy link
Contributor Author

gilescm commented May 23, 2020

Hi, thank you!

Okay cool, I can close this PR and make a new one from my fork's master branch to your master branch. In answer to your questions:

  1. Yes good point, I not happy with the isSnackBar name either. It could be renamed to position? Which could be refactored to take a NotificationPosition enum, which would default to NotificationPosition.top and could then take NotificationPosition.bottom.

  2. I'm not sure what you mean by compatible exactly. The app I have used this in has a bottom navigation bar and the BottomSlideNotification pops up over it, rather than above. This is similar behaviour to the TopSlideNotification which pops up over an AppBar, rather than below. I can investigate an implementation that would allow for the notification to appear above if you like? I've attached a gif of the SnackBar behaviour vs BottomSlideNotification behaviour for clarity. (SnackBar appears first and BottomSlideNotification appears when internet connection is lost)

ezgif com-resize

Additionally due to these differences it may better to make more distance between this and the SnackBar as they have slightly different behaviours. When I create the new PR to master I can rename it more simply to "Support for BottomSlideNotification if that works for you?

@boyan01
Copy link
Owner

boyan01 commented May 24, 2020

Yes, you need open another pull request to merge to the master branch.

Yes good point, I not happy with the isSnackBar name either. It could be renamed to position? Which could be refactored to take a NotificationPosition enum, which would default to NotificationPosition.top and could then take NotificationPosition.bottom.

The idea is great, I agree with your.

@gilescm
Copy link
Contributor Author

gilescm commented May 24, 2020

Okay great thanks, I've made the required changes for NotificationPosition. I will close this PR and open a new one shortly.

@smolugu
Copy link

smolugu commented Sep 8, 2020

Hi, I need this functionality too. Can you show the modifications you made to the showSimpleNotification and showOverlayNotification?

Thank you!

@smolugu
Copy link

smolugu commented Sep 8, 2020

Sorry. Got it! Thanks for making the changes.

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.

3 participants