refactor(vue-app): simplify scrollToTop checking #8621
Conversation
clarkdo
requested review from
a team and
atinux
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## dev #8621 +/- ##
=======================================
Coverage 66.02% 66.02%
=======================================
Files 92 92
Lines 4009 4009
Branches 1103 1103
=======================================
Hits 2647 2647
Misses 1095 1095
Partials 267 267
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Co-authored-by: Sébastien Chopin <seb@nuxtjs.com>
| const { options = {} } = Pages[0] | ||
| return options.scrollToTop !== false | ||
| } | ||
| return Pages.some(({ options }) => options && options.scrollToTop) |
There was a problem hiding this comment.
@clarkdo On line 31 here, it should be checking if options.scrollToTop !== false, similar to line 29? Otherwise the default behavior would be false if multiple components match and none specify the scrollToTop property.
I suspect it may be coded this way to return false for child routes but in that case I would think you would want to validate that the from route is included in the to route's matched routes.
There was a problem hiding this comment.
I think this works same as before change, if there’re multiple matched component, we’ll only check if there’s explicit scrollTo set which is different from single component matched scenario.
There was a problem hiding this comment.
@clarkdo You are right. I would argue it is a bug in the prior to your PR as well. Ill create a separate issue.
The docs say child routes dont scroll to top by default but I would assume that only applies when navigating from the parent route which isnt handled.
Types of changes
Description
Follow up #8612
Add shouldScrollToTop in router.scrollBehaviour.js
getMatchedComponentswhen route is not changedChecklist: