-
Notifications
You must be signed in to change notification settings - Fork 9
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
Rebuild mobile navigator without Onsen, and add smooth swipe behaviour #3898
Conversation
Tested on my Pixel -- looking generally good, although backswipe from a horizontally scrolling grid looks like it does not work. |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
+ Minot tweaks and cleanups
# Conflicts: # CHANGELOG.md
direction = distance > 0 ? 'right' : 'left'; | ||
|
||
const scrollableParent = findScrollableParent(event, 'horizontal'); | ||
if (scrollableParent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I dare ask about nested scrollable parents? Seems uncommon, but....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean a scrollable within a scrollable within a swiper... 🤯
Currently, the one you are touching will scroll until the left edge and then the swiper will come into play, skipping the middle scrollable. I suppose we could make it recursive until we are at the top of the DOM...
Do we have a specific need for it? If not I'd vote with live with it until it comes up, especially given that we should try to avoid horizontal scrolling on mobile generally outside of this context
|
||
stack.push(page); | ||
// Re-use existing PageModels where possible | ||
const existingPageModel = i < this.stack.length ? this.stack[i] : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could "simplify" to just this.stack[i], with out-of-bound behavior for javascript arrays
swiper.progress < 1 || !isDraggableEl(scrollableParent, 'right'); | ||
|
||
// During the swiper transition, undo the scrollable parent's internal scroll | ||
// to keep it static. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!
renderMode = 'lazy', | ||
refreshMode = 'onShowLazy' | ||
}: NavigatorConfig) { | ||
super(); | ||
makeObservable(this); | ||
warnIf( | ||
throwIf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could probably be removed now that we have this throwIf on the page itself.
return true; | ||
} | ||
|
||
// Ignore Onsen "swiper" elements created by tab container (even without swiping enabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the onsen swiper elements are now obsolete, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No... these are installed by the Onsen tab container (which supports swiping between tabs, but we have that disabled). Once that goes we can get rid of this.
Tested against Toolbox in Chrome and Safari on iOS
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
develop
branch as of last change.breaking-change
label + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.