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(OnyxMobileNavButton): scroll on overflowing mobile flyout #1530

Merged
merged 37 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
e862400
clean up old style test
BoppLi Jul 5, 2024
a21c804
wip
BoppLi Jul 5, 2024
3cf5e7d
Merge branch 'main' into fix/1483-max-scrolling-nav-bar
BoppLi Jul 8, 2024
d8fdab3
create scrolling space for mobile nav button
BoppLi Jul 8, 2024
083cbd4
fix paddings
BoppLi Jul 8, 2024
a722556
change style for mobile app version
BoppLi Jul 8, 2024
9847960
clean up
BoppLi Jul 8, 2024
f52a496
add css vars
BoppLi Jul 8, 2024
e15c043
fix center alignment with nav button
BoppLi Jul 8, 2024
239b721
move height to css var in mobile nav button, add scrollable storybook…
BoppLi Jul 8, 2024
d2b0f3f
move ownership of mobile specific styles
BoppLi Jul 8, 2024
9d29e46
add storybook examples
BoppLi Jul 8, 2024
103b24b
playwright test
BoppLi Jul 8, 2024
7c125eb
align component class name
BoppLi Jul 8, 2024
a7292f7
fix screenshots
BoppLi Jul 9, 2024
b1bd9d7
chore: update Playwright screenshots (#1532)
github-actions[bot] Jul 9, 2024
64a0738
docs(changeset): fix(OnyxMobileNavButton): scroll on overflowing mobi…
BoppLi Jul 9, 2024
bde22b4
update changeset
BoppLi Jul 9, 2024
03175de
clean up
BoppLi Jul 9, 2024
e216b30
Merge branch 'main' into fix/1483-max-scrolling-nav-bar
BoppLi Jul 9, 2024
3b78dce
Merge branch 'main' into fix/1483-max-scrolling-nav-bar
BoppLi Jul 9, 2024
e73f0b2
fix missing padding-top
BoppLi Jul 10, 2024
2136a83
chore: update Playwright screenshots (#1542)
github-actions[bot] Jul 10, 2024
75ec506
fix spacing
BoppLi Jul 10, 2024
e1a305b
Apply suggestions from code review
BoppLi Jul 10, 2024
1d2755c
Update .changeset/modern-feet-repeat.md
BoppLi Jul 10, 2024
be15844
Merge branch 'fix/1483-max-scrolling-nav-bar' of https://github.com/S…
BoppLi Jul 10, 2024
ba2dcad
move mobile style to layout component
BoppLi Jul 10, 2024
b4a71b2
Merge branch 'main' into fix/1483-max-scrolling-nav-bar
BoppLi Jul 10, 2024
5384613
set highest breakpoint for nav item max example
BoppLi Jul 10, 2024
290ff77
use prop for headline, not slot
BoppLi Jul 10, 2024
95cbfe0
Merge branch 'main' into fix/1483-max-scrolling-nav-bar
BoppLi Jul 10, 2024
5306b9d
Update packages/sit-onyx/src/components/OnyxMobileNavButton/OnyxMobil…
BoppLi Jul 11, 2024
0e438c1
Update packages/sit-onyx/src/components/OnyxNavBar/OnyxNavBar.stories.ts
BoppLi Jul 11, 2024
cd0f7c5
remove extra class
BoppLi Jul 11, 2024
6a65f3a
Merge branch 'fix/1483-max-scrolling-nav-bar' of https://github.com/S…
BoppLi Jul 11, 2024
bcc4e0a
Merge branch 'main' into fix/1483-max-scrolling-nav-bar
larsrickert Jul 11, 2024
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
10 changes: 10 additions & 0 deletions .changeset/modern-feet-repeat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
"sit-onyx": patch
---

fix(OnyxMobileNavButton): scroll on overflowing mobile flyout

The flyout of OnyxMobileNavButton now has a max height and is scrollable if too many nav/context items exist.

- app version inside the mobile flyout is not positioned absolute anymore and is a disabled list item
- fixed duplicate border in mobile context menu when multiple list items exist
4 changes: 2 additions & 2 deletions apps/demo-app/src/views/HomeView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ const tableData = [
/>

<template v-if="show('OnyxTable')">
<OnyxTable style="max-height: 250px" with-page-scrolling>
<OnyxTable with-page-scrolling>
<template #head>
<tr>
<th v-for="col in tableColumns" :key="col">{{ col }}</th>
Expand Down Expand Up @@ -333,7 +333,7 @@ const tableData = [
gap: var(--onyx-spacing-md);
padding: var(--onyx-spacing-md);
border-right: var(--onyx-1px-in-rem) solid var(--onyx-color-base-neutral-300);
height: calc(100% - var(--onyx-spacing-xl));
height: calc(100%);
width: 16rem;
}
.page {
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -1,14 +1,41 @@
import { test } from "../../playwright/a11y";
import { expect, test } from "../../playwright/a11y";
import { executeMatrixScreenshotTest, mockPlaywrightIcon } from "../../playwright/screenshots";
import OnyxMobileNavButton from "./OnyxMobileNavButton.vue";

test("should render without errors", async ({ mount, makeAxeBuilder }) => {
await mount(
<OnyxMobileNavButton
label="Label"
icon={mockPlaywrightIcon}
open
// playwright sets 8px on the body which interfers with the positioning of the flyout
style="--top-position: 4rem"
>
Lorem ipsum dolor sit amet consectetur. Non in felis erat velit consectetur. Sed integer non
hac viverra nibh vehicula risus ultrices. Molestie cras lobortis vitae gravida et ut. Turpis
nisl pharetra amet ante eu sagittis sit elementum ut.
</OnyxMobileNavButton>,
);

// accessibility tests
const accessibilityScanResults = await makeAxeBuilder().analyze();
expect(accessibilityScanResults.violations, "should pass accessibility checks").toEqual([]);
});

test.describe("Screenshot tests", () => {
executeMatrixScreenshotTest({
name: "Mobile nav button",
columns: ["default", "open"],
rows: ["default", "hover", "active", "focus-visible"],
disablePadding: true,
component: (column) => (
<OnyxMobileNavButton label="Label" icon={mockPlaywrightIcon} open={column === "open"}>
<OnyxMobileNavButton
label="Label"
icon={mockPlaywrightIcon}
open={column === "open"}
// playwright sets 8px on the body which interfers with the positioning of the flyout
style="--top-position: 4rem"
>
Flyout slot content...
</OnyxMobileNavButton>
),
Expand All @@ -20,6 +47,12 @@ test.describe("Screenshot tests", () => {
await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2);
await page.mouse.down();
}

await page.setViewportSize({ width: 600, height: 850 });
await component.evaluate((element) => {
element.style.height = "150px";
element.style.width = "200px";
});
},
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,16 @@ const meta: Meta<typeof OnyxMobileNavButton> = {
icon: defineIconSelectArgType(),
default: { control: { type: "text" } },
},
decorators: [
(story) => ({
components: { story },
template: `<div style="min-height: 10rem;"> <story /> </div>`,
}),
],
}),
parameters: {
layout: "fullscreen",
},
};

export default meta;
Expand All @@ -36,3 +45,15 @@ export const Open = {
open: true,
},
} satisfies Story;

/**
* Example of a mobile nav button with a long content.
* We recommended to watch this example in its [story page](?path=/story/support-mobilenavbutton--scrollable)
* where the full page behavior can be seen.
*/
export const Scrollable: Story = {
args: {
...Open.args,
default: Array.from({ length: 200 }, (_, index) => `Lorem ipsum dolor ${index} `),
},
};
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<script lang="ts" setup>
import x from "@sit-onyx/icons/x.svg?raw";
import OnyxHeadline from "../OnyxHeadline/OnyxHeadline.vue";
import OnyxIcon from "../OnyxIcon/OnyxIcon.vue";
import type { OnyxMobileNavButtonProps } from "./types";

Expand All @@ -16,35 +17,40 @@ const emit = defineEmits<{

defineSlots<{
/**
* Slot for the menu content when its open.
* Slot for the menu content when it's open.
*/
default(): unknown;
}>();
</script>

<template>
<div>
<div class="onyx-mobile-nav-button">
<button
type="button"
class="onyx-mobile-nav-button"
:class="{ 'onyx-mobile-nav-button--active': props.open }"
class="onyx-mobile-nav-button__trigger"
:class="{ 'onyx-mobile-nav-button__trigger--active': props.open }"
:aria-label="props.label"
@click="emit('update:open', !props.open)"
>
<OnyxIcon :icon="props.open ? x : props.icon" />
</button>

<div v-if="props.open" class="onyx-mobile-nav-button__menu">
<div class="onyx-mobile-nav-button__content">
<slot></slot>
</div>
<template v-if="props.open">
<div class="onyx-mobile-nav-button__flyout">
<div class="onyx-mobile-nav-button__menu">
<OnyxHeadline is="h2" v-if="props.headline" class="onyx-mobile-nav-button__headline">
{{ props.headline }}
</OnyxHeadline>

<slot></slot>
</div>
</div>
<div
class="onyx-mobile-nav-button__backdrop"
role="presentation"
@click="emit('update:open', false)"
></div>
</div>
></div
></template>
</div>
</template>

Expand All @@ -53,38 +59,60 @@ defineSlots<{

.onyx-mobile-nav-button {
@include layers.component() {
display: flex;
background-color: var(--onyx-color-base-background-blank);
color: var(--onyx-color-text-icons-neutral-medium);
padding: var(--onyx-spacing-md);
cursor: pointer;

:where(&) {
border: none;
}
// should be adjusted to the height of the control button
--top-position: 3.5rem;
larsrickert marked this conversation as resolved.
Show resolved Hide resolved
$mobile-children-selector: ":has(.onyx-nav-button__mobile-children--open)";

&:hover:not(&--active) {
background-color: var(--onyx-color-base-background-tinted);
}
&__trigger {
display: flex;
background-color: var(--onyx-color-base-background-blank);
color: var(--onyx-color-text-icons-neutral-medium);
padding: var(--onyx-spacing-md);
cursor: pointer;

:where(&) {
border: none;
}

&:hover:not(&--active) {
background-color: var(--onyx-color-base-background-tinted);
}

&:focus-visible {
background-color: var(--onyx-color-base-secondary-100);
outline: none;
&:focus-visible {
background-color: var(--onyx-color-base-secondary-100);
outline: none;
}

&:active,
&--active {
background-color: var(--onyx-color-base-secondary-100);
color: var(--onyx-color-text-icons-secondary-intense);
}
}

&:active,
&--active {
background-color: var(--onyx-color-base-secondary-100);
color: var(--onyx-color-text-icons-secondary-intense);
&__headline {
position: sticky;
top: 0;
z-index: 1;
background-color: var(--onyx-color-base-background-tinted);
padding: var(--onyx-spacing-xl) 0 var(--onyx-spacing-2xs);
margin-inline: auto;
}

&__menu {
&__flyout {
max-height: calc(100vh - var(--top-position) - var(--onyx-spacing-3xl));
width: 100%;
background-color: var(--onyx-color-base-background-tinted);
box-shadow: var(--onyx-shadow-medium-bottom);
position: relative;
font-family: var(--onyx-font-family);
color: var(--onyx-color-text-icons-neutral-intense);
overflow-y: auto;

position: absolute;
top: var(--top-position);
left: 0;
z-index: var(--onyx-z-index-navigation);
}

&__backdrop {
Expand All @@ -93,17 +121,42 @@ defineSlots<{
width: 100%;
height: 100vh;
display: block;
position: absolute;
cursor: pointer;

position: fixed;
top: var(--top-position);
left: 0;
z-index: var(--onyx-z-index-page-overlay);
}

&__content {
max-width: 34rem;
padding: var(--onyx-spacing-xl) var(--onyx-spacing-md);
display: flex;
flex-direction: column;
&__menu {
max-width: 36rem;
padding: 0 var(--onyx-spacing-md) var(--onyx-spacing-xl);
margin-inline: auto;
gap: var(--onyx-spacing-2xs);

&:has(.onyx-nav-bar__mobile-context-content) {
// context menu needs extra padding on top since there is no headline
padding-top: var(--onyx-spacing-xl);
}

// hide all other nav items when nav item with children is open
#{$mobile-children-selector} {
> .onyx-nav-button:not(#{$mobile-children-selector}) {
display: none;
}
}
}

&#{$mobile-children-selector} {
// hide "Navigation" headline when nav item with children is open
.onyx-mobile-nav-button__headline {
display: none;
}
}
// fill up the padding-top if there is no headline
&#{$mobile-children-selector} .onyx-mobile-nav-button__menu,
&__menu:not(:has(.onyx-mobile-nav-button__headline)) {
padding-top: var(--onyx-spacing-xl);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,8 @@ export type OnyxMobileNavButtonProps = {
* If `true`, an "x" icon will be displayed.
*/
open?: boolean;
/**
* Headline of the mobile flyout
*/
headline?: string;
};
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import browserTerminal from "@sit-onyx/icons/browser-terminal.svg?raw";
import placeholder from "@sit-onyx/icons/placeholder.svg?raw";
import search from "@sit-onyx/icons/search.svg?raw";
import settings from "@sit-onyx/icons/settings.svg?raw";
import { defineStorybookActionsAndVModels } from "@sit-onyx/storybook-utils";
import type { Decorator, Meta, StoryObj } from "@storybook/vue3";
import { h } from "vue";
Expand All @@ -9,6 +10,7 @@ import OnyxBadge from "../OnyxBadge/OnyxBadge.vue";
import OnyxIcon from "../OnyxIcon/OnyxIcon.vue";
import OnyxIconButton from "../OnyxIconButton/OnyxIconButton.vue";
import OnyxTag from "../OnyxTag/OnyxTag.vue";
import OnyxMenuItem from "./modules/OnyxMenuItem/OnyxMenuItem.vue";
import OnyxNavButton from "./modules/OnyxNavButton/OnyxNavButton.vue";
import OnyxNavItem from "./modules/OnyxNavItem/OnyxNavItem.vue";
import OnyxNavSeparator from "./modules/OnyxNavSeparator/OnyxNavSeparator.vue";
Expand Down Expand Up @@ -134,3 +136,41 @@ export const WithCustomAppArea = {
appArea: () => [h(OnyxIcon, { icon: placeholder, color: "secondary" }), "Custom name"],
},
} satisfies Story;

/**
* This nav bar has a lot of menu and context area items.
* Both the nav area as well as the context area will overflow when opened.
*/
export const WithOverflowingMobileContent = {
BoppLi marked this conversation as resolved.
Show resolved Hide resolved
args: {
...WithContextArea.args,
mobileBreakpoint: "xl",
default: () => [
h(OnyxNavButton, { label: "Item 1", href: "/" }),
h(
OnyxNavButton,
{ label: "Item 2", href: "/test" },
{
default: () => ["Item 2", h(OnyxBadge, { dot: true, color: "warning" })],
children: () =>
Array.from({ length: 20 }, (_, index) =>
h(OnyxNavItem, { label: `Nested item 2.${index + 1}`, href: "#" }),
),
},
),
Array.from({ length: 20 }, (_, index) =>
h(OnyxNavButton, { label: `Item ${index + 3}`, href: "/" }),
),
],
contextArea: () => [
h(OnyxTag, { label: "QA stage", color: "warning", icon: browserTerminal }),
h(OnyxNavSeparator),
h(OnyxUserMenu, OnyxUserMenuDefault.args, {
default: Array.from({ length: 20 }, (_, index) =>
h(OnyxMenuItem, () => [h(OnyxIcon, { icon: settings }), `Context option ${index}`]),
),
footer: OnyxUserMenuDefault.args.footer,
}),
],
},
} satisfies Story;
Loading
Loading