DRAFT: Hide menus on scroll #257
Conversation
|
I just took a quick look at the code, haven't ran it myself yet, but it seems like it's okay for now. Just a couple of points:
|
|
The commit I just added prints out NOTE: Hot reload won't work for the print out since it's in the state. You'll need a full reload ( |
|
I also think we could consider switching away from the |
|
As example, converting from |
|
Honestly this is pretty amazing, it's performing fantastically and was only a few lines of code change. I will make a new PR now, and revise this one once you take a look at that (as this should still apply). |
|
Sorry, I've been really busy with work and other stuff today so I couldn't reply earlier but wow, that looks really good for a test! I'll reply with some more details when I can but first glance, I think switching to MasonryGridView seems to be the way to go, especially since it supports columns natively I can do some profiling when I have the chance to double check for memory issues or performance regressions but good work! |
|
No need to apologize, you should see how slow I am on my own repos 😉 |
|
@Fmstrat Do you still consider this to be a draft, or should I take another look at the code? |
| @@ -12,6 +12,7 @@ import 'package:thunder/community/widgets/post_card_list.dart'; | |||
| import 'package:thunder/core/auth/bloc/auth_bloc.dart'; | |||
| import 'package:thunder/shared/error_message.dart'; | |||
| import 'package:thunder/shared/sort_picker.dart'; | |||
| import 'dart:developer'; | |||
There was a problem hiding this comment.
Is there a function which requires this package? This seems oddly specific
| if (globalKey.currentState!.innerController.position.pixels >= globalKey.currentState!.innerController.position.maxScrollExtent * 0.7) { | ||
| print('endScroll'); | ||
| // How do we call this from here? | ||
| // context.read<CommunityBloc>().add(GetCommunityPostsEvent(communityId: widget.communityId)); |
There was a problem hiding this comment.
Unfortunately, we won't be able to call this function in the initState function since it has no access to the context.
One "hacky" way to pass in the context is to allow CommunityPage to accept a BuildContext as one of its parameters and then use that context here. However, I'm not sure if that's the best way to go about it
|
I will take a look when I can, but yes, this is definitely still draft. It will need to be merged into the new 2-column code, and of course the review items above are still outstanding issues. I also think we need to find a way to hide the bottom bar as well as the top bar. |
|
Sounds good! Let me know if you want me to help with some of this |
If you feel like it, go for it! I'm pretty busy right now and will be traveling next week. |
|
Personally I dislike menus auto-hiding on scroll, though I feel like I'm in the minority. It would be good if this behaviour could be optional. |
@tom-james-watson Hey, there's two of us! The developer had previously suggested this could be implemented as a toggle. |
|
I'll close this PR as it's changes are no longer relevant because of the refactor of the feed page. With the refactor, it should be much easier now to implement a toggle which automatically hides the top bar on scroll! |
@hjiangsu This is a very first-draft of menu hiding on scroll. There are some commented items that I'm not sure of the full impact:
These were required to make the hiding work, but I'm unfamiliar with Dart and just doing this in spare time ;)
I also can't test horizontal swipes in the Android Emulator for some reason. Lastly, I didn't focus on the bottom bar yet as I'd like to know if it all works for you with horizontal swipes, etc.
Addresses: #74