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

Refactor snackbars to use overlays and remove dependency on context #1111

Merged
merged 12 commits into from
Feb 8, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Feb 2, 2024

Pull Request Description

This is a draft PR containing my attempt at refactoring the usage of snackbars. There are two main concerns with the current usage of snackbars:

  • The current snackbars are heavily dependent on Scaffolds which means that they only display on the Scaffold that they are called upon. This causes an issue where a snackbar does not appear correctly because it is somewhere farther down the navigation stack.
  • Calling snackbars on the proper page/Scaffold added a lot of extra code which made the code more confusing. This is doubled by the fact that there are times where the same snackbar can appear on different Scaffolds (depending on the context)

My solution for this is to use an alternate method of showing "snackbars". Instead of using Flutter's Snackbar (which is highly dependent on having a Scaffold), I've switched over to using an overlay widget which renders a snackbar-like widget. While this widget does not fully match the material 3 design specification for snackbars, I did attempt to make it as similar as possible (with the possibility to improve it in the future!)

As this is an overlay, the snackbar shows on top of all the other widgets. This means that the FAB, or any other content, will be behind the snackbar.

With this change, it solves the two concerns mentioned above:

  • We can call showSnackbar without any context, and it should pop up properly
  • The snackbar is an overlay, which means that it will show on top of all the other widgets on the screen. This allows us to show information when we need to without the worry of Scaffolds.

Note: A lot of the changes made were to remove context and any additional parameters that were no longer needed (scaffoldMessengerKey). The main chunk of changes are contained within lib/shared/snackbar.dart.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Quick Demo (more to be shown after some more testing)

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-02.at.15.04.57.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@micahmo
Copy link
Member

micahmo commented Feb 3, 2024

I just wanted to say thank you so much for taking this on to address some of the concerns I raised. I really appreciate it, and this seems like a perfect approach. I look forward to testing and reviewing the final product.

P.S. Can the overlay be dismissed with a swipe? If not, that could be added later. Just curious.

Again, thank you so much!

@hjiangsu
Copy link
Member Author

hjiangsu commented Feb 5, 2024

Can the overlay be dismissed with a swipe?

It can! You can dismiss it with a downward swipe. I think the only caveat with the implementation so far is that I'm finding it difficult to forcefully "hide" the snackbar. If I show another snackbar, the previous one closes and the new one shows up. But I can't seem to close shown snackbars prematurely. I'm not sure how big of an issue this would be, but judging from the code, it seems like a very rare use-case.

@micahmo
Copy link
Member

micahmo commented Feb 5, 2024

If I show another snackbar, the previous one closes and the new one shows up. But I can't seem to close shown snackbars prematurely.

I think that's perfectly reasonable. As long as new ones replace old ones and they can be dismissed by the user, I think we should be all set. Thanks again for working on this!

Copy link
Member Author

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

Alright, I've done some more testing to see if this implementation works with our usage of snackbars!

@micahmo If you'd like, you can give this a try and provide any feedback (positioning, colours, etc) or let me know of any situations that come up which should be fixed!

lib/post/utils/navigate_create_post.dart Show resolved Hide resolved
@hjiangsu hjiangsu marked this pull request as ready for review February 5, 2024 17:49
@micahmo
Copy link
Member

micahmo commented Feb 5, 2024

This is so awesome, thanks for taking the time to do this! I will take it for a test run when I can!

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

The code LGTM! Love the cleanup!

We could almost publish our snackbar as a package. 😆

@micahmo
Copy link
Member

micahmo commented Feb 5, 2024

I took this for a test run and everything worked really well. I think the appearance is perfect.

I did find a couple of edge cases (to be addressed later or ignored 😆).

  • I found that the snackbar can appear below the keyboard. (For example, create a new post with an error and try to publish it while the keyboard is open.) I'm not sure whether we can calculate the height and put it above, or force close the keyboard in that case, or maybe it doesn't matter.
  • I found that if you manually dismiss the snackbar and then perform an action which creates an overlay, it can come back. Video below.
qemu-system-x86_64_3BDaGnPR3U.mp4

That's all for now! Maybe more as I daily drive it. I can't thank you enough for tackling this one. One of my biggest pet peeves as you can probably guess. 😆

@hjiangsu
Copy link
Member Author

hjiangsu commented Feb 5, 2024

Thanks for catching those issues!

I found that the snackbar can appear below the keyboard.

This is the logic I'm using at the moment to determine the height (padding) of the container. I'm thinking I might have to add MediaQuery.of(context).viewInsets.bottom to work around the keyboard issue.

EdgeInsets.only(bottom: MediaQuery.of(context).viewPadding.bottom + kBottomNavigationBarHeight + singleLineVerticalPadding),

I found that if you manually dismiss the snackbar and then perform an action which creates an overlay, it can come back.

I noticed that during my initial testing too, but I thought I squashed that bug 😆. I think this has to do with Dismissible and the onDismiss behaviour not working as expected. I'll look into it!

That's all for now! Maybe more as I daily drive it. I can't thank you enough for tackling this one.

Yup, no worries!

As a side note, I did find it weird that the Snackbar was dependent on Scaffolds at the beginning, but it made a bit more sense when I dived into the Flutter code. The main reason has to do with the height calculation of the Snackbar itself since it needs to consider the bottom nav bar, and the FAB (as per material guidelines here).

@hjiangsu
Copy link
Member Author

hjiangsu commented Feb 5, 2024

Just pushed a commit to hopefully fix both the issues you mentioned! One quirk is that when the snackbar pops up with the keyboard, and you dismiss the keyboard, the snackbar will stay in the same position (since it doesnt recalculate the height). I might try to find a way to animate it down but it might take a bit more work.

Let me know how the changes are!

@micahmo
Copy link
Member

micahmo commented Feb 5, 2024

I did find it weird that the Snackbar was dependent on Scaffolds at the beginning, but it made a bit more sense when I dived into the Flutter code. The main reason has to do with the height calculation of the Snackbar itself since it needs to consider the bottom nav bar, and the FAB

That makes sense! Although on the other hand, even with those calculations, the behavior was unpredictable. Like on the home page it would appear above the bottom NavigationBar, but on other pages it would fill the whole bottom of the screen, including the OS gesture area. And sometimes the FAB would move out of the way, but sometimes it wouldn't (like on a pushed community page). Anyway, I think we are making an overall improvement. 😊

Just pushed a commit to hopefully fix both the issues you mentioned!

Just tested and it looks great! I think this is good to go!

@hjiangsu
Copy link
Member Author

hjiangsu commented Feb 6, 2024

I'll go ahead and merge this in, and close #1102? If there are more issues or adjustments to be made, we can make those in a separate PR!

@micahmo
Copy link
Member

micahmo commented Feb 6, 2024

Sounds good!

@hjiangsu hjiangsu merged commit c1b650b into develop Feb 8, 2024
1 check passed
@hjiangsu hjiangsu deleted the refactor/snackbar-overlay branch February 8, 2024 01:32
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.

None yet

2 participants