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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Sep 13, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49fa003
fix(MobilePortraitNav): use single grid layout to prevent scroll posi…
Sep 8, 2022
1137fa4
fix(MobilePortraitNav): display views over header & footer to prevent…
Sep 9, 2022
9487349
style: fix unintended whitespace changes
Sep 9, 2022
259ad04
fix: use appropriate mobile-portrait breakpoint, remove redundant wid…
Sep 9, 2022
46803c3
refactor: use minimal properties to define mobile portrait view styling
Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions assets/css/_nav.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@
gap: 0;
grid-template-columns: max-content minmax(0, 1fr);
grid-template-rows: max-content minmax(0, 1fr);
grid-template-areas:
"top top"
"left content";
grid-template-areas: "top top" "left content";
width: 100%;
height: 100%;
}
Expand All @@ -25,19 +23,11 @@
gap: 0;
grid-template-columns: minmax(0, 1fr);
grid-template-rows: max-content minmax(0, 1fr) max-content;
grid-template-areas:
"top"
"content"
"bottom";
grid-template-areas: "top" "content" "bottom";
width: 100%;
height: 100%;
}

.m-nav--narrow.m-nav--covered {
grid-template-rows: minmax(0, 1fr);
grid-template-areas: "content";
}

.m-nav__nav-bar {
z-index: 1100;
}
Expand Down
9 changes: 7 additions & 2 deletions assets/css/_notification_drawer.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,16 @@
top: 0;
width: 23.5rem;
z-index: 1001;

@media screen and (max-width: $mobile-max-width) {
width: 100%;
}
@media screen and (max-width: $mobile-max-width) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

position: fixed;
top: 0;
bottom: 0;
width: 100%;
Copy link
Member

Choose a reason for hiding this comment

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

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.

overflow-y: scroll;
}
}

.m-notification-drawer__content {
Expand All @@ -32,7 +38,6 @@
cursor: pointer;
position: absolute;
right: 0;

&:hover {
font-weight: 700;
}
Expand Down
9 changes: 7 additions & 2 deletions assets/css/_properties_panel.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,19 @@
top: 0;
width: 23.5rem;
z-index: 1002;

@media screen and (max-width: $mobile-max-width) {
width: 100vw;

.m-nav__app-content & {
width: 100%;
}
}
@media screen and (max-width: $mobile-max-width) {
position: fixed;
top: 0;
bottom: 0;
width: 100%;
Copy link
Member

Choose a reason for hiding this comment

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

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

overflow-y: scroll;
}
}

.m-properties-panel .m-properties-panel__header-wrapper {
Expand Down
30 changes: 7 additions & 23 deletions assets/css/_swings_view.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,28 @@
right: 0;
top: 0;
z-index: 1001;

.m-old-close-button {
padding: 1.5rem;
position: absolute;
right: 0rem;
top: 0rem;
}

// Full width on mobile portrait, to be updated with more responsive designs
@media screen and (max-width: 480px) {
.m-nav__app-content & {
width: 100%;

.m-swings-view__table {
width: 100%;
}
}
}
@media screen and (max-width: $mobile-max-width) {
Copy link
Member

Choose a reason for hiding this comment

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

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

position: fixed;
top: 0;
bottom: 0;
width: 100%;
overflow-y: scroll;
}
}

.m-swings-view__table-container {
Expand Down Expand Up @@ -58,17 +62,14 @@
position: -webkit-sticky;
top: -1px;
z-index: 1002;

.m-swings-view__table-header-cell-subheaders {
display: flex;
justify-content: flex-end;

.m-swings-view__table-header-cell-subheader {
padding-top: 0.125rem;
font-size: 0.6875rem;
font-weight: 400;
}

.m-swings-view__table-header-cell-route-subheader {
display: inline-block;
padding-top: 0.125rem;
Expand All @@ -77,15 +78,12 @@
font-weight: 400;
}
}

&.m-swings-view__table-header-cell-swing-on {
vertical-align: bottom;
}

&.m-swings-view__table-header-cell-swing-off {
padding-left: 3.0625rem;
}

&:last-child {
padding-right: 1.1875rem;
}
Expand Down Expand Up @@ -118,17 +116,14 @@
font-size: 0.6875rem;
font-variant-numeric: tabular-nums;
text-align: right;

&:last-child {
padding-left: 1.0625rem;
padding-right: 1.1875rem;
}

.m-swings-view__table-cell-contents {
display: flex;
justify-content: flex-end;
align-items: center;

button {
text-decoration-line: underline;
}
Expand All @@ -141,11 +136,9 @@
color: $color-primary;
padding-top: 0.6875rem;
padding-bottom: 0.6875rem;

&.m-swings-view__show-past-enabled {
border-bottom: 1px solid #dcdcdc;
}

&.m-swings-view__show-past-disabled {
border-bottom: 1px solid #515459;
}
Expand All @@ -156,7 +149,6 @@
display: inline-block;
padding-left: 0.5rem;
padding-right: 0.375rem;

svg {
fill: $color-primary;
stroke: $color-primary;
Expand Down Expand Up @@ -185,24 +177,20 @@
display: flex;
justify-content: center;
align-items: center;

&.m-swings-view__run-icon-arrow {
padding-right: 0.375rem;
stroke: $color-primary-dark;
width: 0.5rem;
height: 0.5rem;

svg {
width: 0.5rem;
height: 0.5rem;
}
}

&.m-swings-view__run-icon-ghost {
padding-right: 0.375rem;
width: 1.1875rem;
height: 1.1875rem;

svg {
width: 1.1875rem;
height: 1.1875rem;
Expand All @@ -214,21 +202,17 @@
.m-swings-view__header {
padding-left: 1.75rem;
}

.m-swings-view__description {
padding-left: 1.75rem;
}

.m-swings-view__table-container {
padding-left: 0rem;
padding-right: 0rem;
}

.m-swings-view__table-header-cell {
&.m-swings-view__table-header-cell-swing-off {
padding-left: 3rem;
}

&.m-swings-view__table-header-cell-swing-on {
padding-left: 1.5625rem;
}
Expand Down
2 changes: 0 additions & 2 deletions assets/css/properties_panel/_vehicle_properties_panel.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
margin-top: 1rem;
min-height: 320px;
z-index: 9;

.m-vehicle-map {
flex: 1 0 auto;
}
Expand All @@ -71,7 +70,6 @@

.m-vehicle-properties-panel__data-discrepancy-source-id {
font-style: italic;

&::after {
content: ": ";
}
Expand Down
54 changes: 29 additions & 25 deletions assets/src/components/nav/mobilePortraitNav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,33 +19,37 @@ const MobilePortraitNav = ({
const { mobileMenuIsOpen, routeTabs, openView, selectedVehicleOrGhost } =
state

if (openView !== OpenView.None || selectedVehicleOrGhost) {
return (
<div className="m-nav--narrow m-nav--covered">
<div className="m-nav__app-content">{children}</div>
const isViewOpen = openView !== OpenView.None || selectedVehicleOrGhost

const navVisibilityStyle = isViewOpen ? "hidden" : "visible"

return (
<div className="m-nav--narrow">
<div
className="m-nav__nav-bar m-nav__nav-bar--top"
style={{ visibility: navVisibilityStyle }}
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 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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

>
<TopNavMobile
toggleMobileMenu={() => dispatch(toggleMobileMenu())}
openNotificationDrawer={() => dispatch(openNotificationDrawer())}
routeTabs={routeTabs}
mobileMenuIsOpen={mobileMenuIsOpen}
/>
</div>
)
} else {
return (
<div className="m-nav--narrow">
<div className="m-nav__nav-bar m-nav__nav-bar--top">
<TopNavMobile
toggleMobileMenu={() => dispatch(toggleMobileMenu())}
openNotificationDrawer={() => dispatch(openNotificationDrawer())}
routeTabs={routeTabs}
mobileMenuIsOpen={mobileMenuIsOpen}
/>
</div>
<div className="m-nav__app-content">{children}</div>
<div className="m-nav__nav-bar m-nav__nav-bar--bottom">
<BottomNavMobile
mobileMenuIsOpen={mobileMenuIsOpen}
openSwingsView={() => dispatch(openSwingsView())}
/>
</div>

<div className="m-nav__app-content">{children}</div>

<div
className="m-nav__nav-bar m-nav__nav-bar--bottom"
style={{ visibility: navVisibilityStyle }}
>
<BottomNavMobile
mobileMenuIsOpen={mobileMenuIsOpen}
openSwingsView={() => dispatch(openSwingsView())}
/>
</div>
)
}
</div>
)
}

export default MobilePortraitNav
18 changes: 10 additions & 8 deletions assets/tests/components/nav/mobilePortraitNav.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import React from "react"
import { render } from "@testing-library/react"
import "@testing-library/jest-dom"

import MobilePortraitNav from "../../../src/components/nav/mobilePortraitNav"
import { StateDispatchProvider } from "../../../src/contexts/stateDispatchContext"
import { initialState, OpenView } from "../../../src/state"
Expand All @@ -17,8 +19,8 @@ describe("MobilePortraitNav", () => {
</StateDispatchProvider>
)

expect(result.queryByTitle("Swings View")).not.toBeNull()
expect(result.queryByTitle("Notifications")).not.toBeNull()
expect(result.queryByTitle("Swings View")).toBeVisible()
expect(result.queryByTitle("Notifications")).toBeVisible()
})

test("doesn't render top / bottom nav when a view is open", () => {
Expand All @@ -34,8 +36,8 @@ describe("MobilePortraitNav", () => {
</StateDispatchProvider>
)

expect(result.queryByTitle("Swings View")).toBeNull()
expect(result.queryByTitle("Notifications")).toBeNull()
expect(result.queryByTitle("Swings View")).not.toBeVisible()
expect(result.queryByTitle("Notifications")).not.toBeVisible()
})

test("doesn't render top / bottom nav when a vehicle is selected", () => {
Expand All @@ -54,8 +56,8 @@ describe("MobilePortraitNav", () => {
</StateDispatchProvider>
)

expect(result.queryByTitle("Swings View")).toBeNull()
expect(result.queryByTitle("Notifications")).toBeNull()
expect(result.queryByTitle("Swings View")).not.toBeVisible()
expect(result.queryByTitle("Notifications")).not.toBeVisible()
})

test("doesn't render top / bottom nav when notification drawer is open", () => {
Expand All @@ -74,7 +76,7 @@ describe("MobilePortraitNav", () => {
</StateDispatchProvider>
)

expect(result.queryByTitle("Swings View")).toBeNull()
expect(result.queryByTitle("Notifications")).toBeNull()
expect(result.queryByTitle("Swings View")).not.toBeVisible()
expect(result.queryByTitle("Notifications")).not.toBeVisible()
})
})