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

fix(vue-app): check for Page.options before looking within #8612

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

danielroe
Copy link
Member

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)

Description

Difficult to reproduce the issue but the fix is lightweight.

closes #8234

Checklist:

  • All new and existing tests are passing.

@danielroe danielroe self-assigned this Jan 9, 2021
@codecov-io
Copy link

Codecov Report

Merging #8612 (166a025) into dev (3bba4c6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #8612   +/-   ##
=======================================
  Coverage   66.02%   66.02%           
=======================================
  Files          92       92           
  Lines        4009     4009           
  Branches     1103     1103           
=======================================
  Hits         2647     2647           
  Misses       1095     1095           
  Partials      267      267           
Flag Coverage Δ
unittests 66.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bba4c6...166a025. Read the comment docs.

@@ -31,9 +31,9 @@ export default function (to, from, savedPosition) {
// Scroll to the top of the page if...
if (
// One of the children set `scrollToTop`
(Pages.some(Page => Page.options.scrollToTop) ||
(Pages.some(Page => Page.options && Page.options.scrollToTop) ||
Copy link
Member

Choose a reason for hiding this comment

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

Could we use this?

Suggested change
(Pages.some(Page => Page.options && Page.options.scrollToTop) ||
(Pages.some(Page => Page.options?.scrollToTop) ||

Copy link
Member

Choose a reason for hiding this comment

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

This may add extra polyfill if user is not using optional chaining.

Copy link
Member

@clarkdo clarkdo Jan 11, 2021

Choose a reason for hiding this comment

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

Let's merge this and I may make some refactoring to this file after the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scrollToTop throws Page.options is undefined
4 participants