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

Add Swipe support between navbar pages #55

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

bebaoboy
Copy link
Contributor

@bebaoboy bebaoboy commented Jul 16, 2024

  • bool swipeable: set to true to opt-in and be able to swipe horizontally.
    Default set to false
  • Rect swipeableLeftArea and Rect swipeableRightArea: define the areas on the left and right side of the screen where we can swipe back and forth between tabs.
    Default values for each area is 50 pixels wide, about 80% screen height (similar to the code below):
NavbarRouter(
  swipeable: swipeable,
  swipeableLeftArea: Rect.fromLTWH(
      0, 50, 50, MediaQuery.of(context).size.height * 0.9),
  swipeableRightArea: Rect.fromLTWH(
      MediaQuery.of(context).size.width - 50,
      50,
      50,
      MediaQuery.of(context).size.height * 0.8),
 destinations: [..],
...
);
  • Support mobile and (maybe) desktop too.
  • Demo video

  • The two blue bars indicate the area to swipe between pages, we can only swipe to the next/previous page from there. (In production, these bars will not be visible).
  • The horizontal swipe does not hinder other scrollables on the screen (notice the horizontal list, and the slider)
  • Video:
Portrait Landscape
451863553_7879930308750028_6837724918374055852_n.mp4
451586683_8583293638352793_8226882859992401935_n.mp4
  • Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.

  • I listed at least one issue that this PR fixes in the description above.

  • I updated/added relevant documentation (doc comments with ///).

  • I added new tests to check the change I am making, or this PR is [test-exempt].

  • All existing and new tests are passing using flutter test

If you need help, consider pinging the maintainer @maheshmnj

@bebaoboy bebaoboy marked this pull request as draft July 16, 2024 16:46
@maheshj01
Copy link
Owner

maheshj01 commented Jul 17, 2024

@bebaoboy Whenever you are ready mark this PR as ready for review and I will take a look and feel free to ping me anytime if you woud like to discuss anything

@bebaoboy bebaoboy marked this pull request as ready for review July 19, 2024 16:26
@bebaoboy
Copy link
Contributor Author

Im ready for review 😁

example/lib/main.dart Outdated Show resolved Hide resolved
lib/src/navbar_router.dart Show resolved Hide resolved
lib/src/navbar_router.dart Outdated Show resolved Hide resolved
lib/src/navbar_router.dart Outdated Show resolved Hide resolved
lib/src/navbar_router.dart Outdated Show resolved Hide resolved
lib/src/navbar_router.dart Outdated Show resolved Hide resolved
@maheshj01
Copy link
Owner

@bebaoboy I have added my comments let me know what you think?

@bebaoboy bebaoboy changed the title [WIP] Add Swipe support between navbar pages Add Swipe support between navbar pages Jul 25, 2024
@bebaoboy
Copy link
Contributor Author

The idea is not using page view 'cause the content behind pageview cannot be scroll horizontally. Then why not make the PageView NeverScrollable? If I do that, why should I use PageView in the first place if I cannot swipe it🤣 Besides, the PageView and ListView ScrollController are similar so drag logic will be the same.

This is the PR for the design I think of, the idea is using 2 Gesture Detector on the 2 edges on the screen to help user scroll through the pages. No horizontal scrollables will be affected.

Maybe I skimmed too fast through the client spec and missed the point, or the originally said design was too abstract for me. If you don't want to use this design then leave it and maybe make another PR.

@maheshj01
Copy link
Owner

maheshj01 commented Jul 27, 2024

@bebaoboy Do you think you will be able to make that change to land this PR? I think its just that there will be one draggable area, I don't think its a major change in your code.

Just remove one of the Rect and handle these three methods to detect left or right swipe

onHorizontalDragStart: (details) {
    onDragStart(details);
  },
  onHorizontalDragUpdate: (details) {
    onDragUpdate(details);
  },
  onHorizontalDragEnd: (details) {
    onDragEnd(details);
  },

@bebaoboy
Copy link
Contributor Author

Maybe I'll try that.

@maheshj01
Copy link
Owner

Let me know if you have any further questions, I would be happy to discuss.

- only one swipeable area
- refractor some code to gestures.dart
@maheshj01
Copy link
Owner

Hi @bebaoboy, Let me know whenever this is ready for review, No rush take your time, let me know if you have any questions.

Best
Mahesh

@bebaoboy
Copy link
Contributor Author

I think its ready for review @maheshj01

@maheshj01
Copy link
Owner

Great, I will take a look at it this weekeend.

@@ -1793,4 +1795,103 @@ void main() {
await tester.pumpAndSettle();
expect((navItems[2].selectedIcon as Icon).color, Colors.green);
});

group('Swipeable extra test', () {
testWidgets('Should not be able to drag on center', (WidgetTester tester) async {
Copy link
Owner

Choose a reason for hiding this comment

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

By default the draggable area should take the width of available view including center,

Comment on lines +3 to +6
const double kDragAreaTop = 50;
const double kDragAreaWidth = 50;
const double kOpacityWhenSwipeable = 0.5;
const double kDragAreaHeightFactor = 0.8;
Copy link
Owner

Choose a reason for hiding this comment

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

These should be hardcoded to default constants of the visible viewport dimensions

The Defaults should be
kDragAreaTop = 0
kDragAreaWidth = NavbarRouter pages content width

Copy link
Owner

Choose a reason for hiding this comment

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

I would suggest taking a look at the Whatsapp for Android and you can see how the entire view can be scrolled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still don't get it 😂 then what's the point of not using PageView in the first place? I mean... I feel like this PR has strayed too far from my original idea

child: GestureDetector(
key: const ObjectKey("swipe-left"),
behavior: HitTestBehavior.translucent,
onHorizontalDragStart: (details) {
Copy link
Owner

Choose a reason for hiding this comment

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

create separate methods out of the build method handleDragStart,handleDragUpdate, and handleDragEnd and move this logic there and move as much logic as possible in the corresponding methods of Gesture class to keep the build method clean and readable

controller: pageGesture.pageController,
itemBuilder: (context, i) {
// use keep-alive to prevent list builder to rebuild
return KeepAliveWrapper(
Copy link
Owner

Choose a reason for hiding this comment

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

When we horizontally swipe the widget should be visible even if the drag did not complete, currently I see a blank white screen until the drag completes

for instance if you are on index 0 and make start dragLeft and drag lets say 60% without leaving your touch on the screen, I see the tab on index 2 has not yet rendered or visible it should always be visible as you start scrolling.

To better understand this I highly recommend trying out the swipe gesture on Whatsapp app for Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, and I recommend you to take a look at the demo video and see it had been implemented in the first place before you told me to refractor the code somehow.

Comment on lines +270 to +276
// swipeableLeftArea: Rect.fromLTWH(
// 0, 50, 50, MediaQuery.of(context).size.height * 0.9),
// swipeableRightArea: Rect.fromLTWH(
// MediaQuery.of(context).size.width - 50,
// 50,
// 50,
// MediaQuery.of(context).size.height * 0.9),
Copy link
Owner

Choose a reason for hiding this comment

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

remove this comment and add

Suggested change
// swipeableLeftArea: Rect.fromLTWH(
// 0, 50, 50, MediaQuery.of(context).size.height * 0.9),
// swipeableRightArea: Rect.fromLTWH(
// MediaQuery.of(context).size.width - 50,
// 50,
// 50,
// MediaQuery.of(context).size.height * 0.9),
swipeableArea: Rect.fromLTWH(0, 0, MediaQuery.of(context).size.width,

Even without specifying swipeableArea whole width should be swipeable

Copy link
Owner

@maheshj01 maheshj01 left a comment

Choose a reason for hiding this comment

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

@bebaoboy I have added some comments let me know what you think

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.

2 participants