fix(MobilePortraitNav): use single grid layout to prevent scroll position resetting when a view is opened #1719

KaylaBrady · 2022-09-08T18:46:15Z

ticket: https://app.asana.com/0/1148853526253437/1202946731207901/f

The issue seemed to be that when opening the VPP, the page was rendered with a new grid, which seemed to reset the scroll position of the ladder page.

I'm not sure that the approach I've taken here is the cleanest one. My intention was to fix the issue without having to introduce storing the scroll position in state. By using one grid class and conditionally hiding the top / bottom navs, the scroll reset issue is no longer a problem. If there is a cleaner or more conventional approach here I am certainly open to changing!

I also wasn't sure of the best way to introduce a regression test for this - there is already test coverage to make sure the navs are conditionally shown / hidden, but maintaining scroll position seems like it might require using something like Cypress.

…tion resetting when a view is opened

github-actions · 2022-09-08T18:53:40Z

Coverage of commit `49fa003`

Summary coverage rate:
  lines......: 94.0% (2447 of 2604 lines)
  functions..: 72.8% (1030 of 1415 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

… size adjustments of the route ladder

KaylaBrady · 2022-09-09T18:30:56Z

assets/src/components/nav/mobilePortraitNav.tsx

+    <div className="m-nav--narrow">
+      <div
+        className="m-nav__nav-bar m-nav__nav-bar--top"
+        style={{ visibility: navVisibilityStyle }}


I opted to do inline styling here as a work around for this issue w/ jest toBeVisible not working with separate stylesheets unless there is some css configuration work done. If there is a preferred alternative approach I'd be open to trying it!

My usual approach for these things has been to set a class based on the condition in question, and then just assert the presence / absence of the relevant classes on the element in my tests. This works, too, though, and toBeVisible actually tests the relevant property more directly.

github-actions · 2022-09-09T18:33:50Z

Coverage of commit `1137fa4`

Summary coverage rate:
  lines......: 94.0% (2447 of 2604 lines)
  functions..: 72.8% (1030 of 1415 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

github-actions · 2022-09-09T18:50:26Z

Coverage of commit `5971af0`

Summary coverage rate:
  lines......: 94.0% (2447 of 2604 lines)
  functions..: 72.8% (1030 of 1415 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

github-actions · 2022-09-09T18:56:03Z

Coverage of commit `9487349`

Summary coverage rate:
  lines......: 94.0% (2447 of 2604 lines)
  functions..: 72.8% (1030 of 1415 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

lemald · 2022-09-09T19:34:29Z

assets/css/_notification_drawer.scss

@@ -13,6 +13,13 @@
  @media screen and (max-width: $mobile-max-width) {
    width: 100%;
  }
+  @media screen and (max-width: $mobile-max-width) {


Is $mobile-max-width actually the correct value? It's defined in app.scss as 768px and it was a breakpoint used in most old Skate designs, but it's not the same as the breakpoint for mobile portrait nav.

On the other hand, if it is supposed to be $mobile-max-width, this can be combined with the above selector / block.

Good call this is not the right value - as-is, shrinking to a window between 480-768px wide produces a broken mashup of navs. I'll introduce a new $mobile-portrait-max-width variable and reference that instead.

For what it's worth, the breakpoints for the new nav are defined in src/hooks/useDeviceType.ts. Since the nav logic happens largely in JavaScript, I didn't end up creating new SCSS constants for the breakpoints. I think eventually we want to retire the use of the existing $mobile-max-width and make all of the different breakpoints consistent.

lemald · 2022-09-09T19:36:38Z

assets/css/_notification_drawer.scss

+    position: fixed;
+    top: 0;
+    bottom: 0;
+    width: 100%;


It should have the same outcome, but I wonder if 100vw would be a bit clearer about the fact that the intent is to take up all of the horizontal space on the screen. I don't think we're actually consistent one way or another in the existing CSS about when we use vw / vh.

lemald · 2022-09-09T19:37:22Z

assets/css/_properties_panel.scss

+    position: fixed;
+    top: 0;
+    bottom: 0;
+    width: 100%;


Other comments from above apply here, but also note that the width: 100% here is redundant with width: 100vw in the block above.

lemald · 2022-09-09T19:37:58Z

assets/css/_swings_view.scss

@@ -27,6 +27,13 @@
      }
    }
  }
+  @media screen and (max-width: $mobile-max-width) {


See same question as above about $mobile-max-width.

…th properties

github-actions · 2022-09-09T20:10:09Z

Coverage of commit `46803c3`

Summary coverage rate:
  lines......: 94.0% (2447 of 2604 lines)
  functions..: 72.8% (1030 of 1415 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

lemald

👍

github-actions · 2022-09-09T20:24:34Z

Coverage of commit `46803c3`

Summary coverage rate:
  lines......: 94.0% (2447 of 2604 lines)
  functions..: 72.8% (1030 of 1415 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

fix(MobilePortraitNav): use single grid layout to prevent scroll posi…

49fa003

…tion resetting when a view is opened

KaylaBrady temporarily deployed to dev-blue September 8, 2022 18:53 Inactive

fix(MobilePortraitNav): display views over header & footer to prevent…

1137fa4

… size adjustments of the route ladder

KaylaBrady force-pushed the kb-mobile-vpp-scroll-fix branch from a7b6927 to 1137fa4 Compare September 9, 2022 18:27

KaylaBrady commented Sep 9, 2022

View reviewed changes

KaylaBrady force-pushed the kb-mobile-vpp-scroll-fix branch from 6a024ca to 5971af0 Compare September 9, 2022 18:45

style: fix unintended whitespace changes

9487349

KaylaBrady force-pushed the kb-mobile-vpp-scroll-fix branch from 5971af0 to 9487349 Compare September 9, 2022 18:49

KaylaBrady requested a review from lemald September 9, 2022 18:56

KaylaBrady assigned lemald Sep 9, 2022

KaylaBrady temporarily deployed to dev-blue September 9, 2022 19:07 Inactive

lemald reviewed Sep 9, 2022

View reviewed changes

Brady added 2 commits September 9, 2022 16:02

fix: use appropriate mobile-portrait breakpoint, remove redundant wid…

259ad04

…th properties

refactor: use minimal properties to define mobile portrait view styling

46803c3

lemald approved these changes Sep 9, 2022

View reviewed changes

KaylaBrady temporarily deployed to dev-blue September 9, 2022 20:25 Inactive

KaylaBrady temporarily deployed to dev-blue September 12, 2022 18:20 Inactive

KaylaBrady temporarily deployed to dev-blue September 12, 2022 19:26 Inactive

KaylaBrady merged commit f3f2e36 into master Sep 13, 2022

KaylaBrady deleted the kb-mobile-vpp-scroll-fix branch September 13, 2022 16:53

firestack mentioned this pull request Nov 30, 2023

Full-screen panels on small screens #2289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(MobilePortraitNav): use single grid layout to prevent scroll position resetting when a view is opened #1719

fix(MobilePortraitNav): use single grid layout to prevent scroll position resetting when a view is opened #1719

KaylaBrady commented Sep 8, 2022

github-actions bot commented Sep 8, 2022

KaylaBrady Sep 9, 2022

lemald Sep 9, 2022

github-actions bot commented Sep 9, 2022

github-actions bot commented Sep 9, 2022

github-actions bot commented Sep 9, 2022

lemald Sep 9, 2022

KaylaBrady Sep 9, 2022

lemald Sep 9, 2022

lemald Sep 9, 2022

lemald Sep 9, 2022

lemald Sep 9, 2022

github-actions bot commented Sep 9, 2022

lemald left a comment

github-actions bot commented Sep 9, 2022

fix(MobilePortraitNav): use single grid layout to prevent scroll position resetting when a view is opened #1719

fix(MobilePortraitNav): use single grid layout to prevent scroll position resetting when a view is opened #1719

Conversation

KaylaBrady commented Sep 8, 2022

github-actions bot commented Sep 8, 2022

Coverage of commit 49fa003

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2022

Coverage of commit 1137fa4

github-actions bot commented Sep 9, 2022

Coverage of commit 5971af0

github-actions bot commented Sep 9, 2022

Coverage of commit 9487349

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2022

Coverage of commit 46803c3

lemald left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2022

Coverage of commit 46803c3

Coverage of commit `49fa003`

Coverage of commit `1137fa4`

Coverage of commit `5971af0`

Coverage of commit `9487349`

Coverage of commit `46803c3`

Coverage of commit `46803c3`