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

fix(menu): menu no longer disappears with multiple split panes #28370

Merged
merged 7 commits into from
Oct 19, 2023
Merged

fix(menu): menu no longer disappears with multiple split panes #28370

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
03c9f9b
test(split-pane): add base test setup
liamdebeasi Oct 17, 2023
2ef1f1e
add failing test
liamdebeasi Oct 17, 2023
30ecadb
fix test
liamdebeasi Oct 17, 2023
d4377c2
add more menus
liamdebeasi Oct 17, 2023
ac87d08
fix(menu): menu no longer disappears with multiple split panes
liamdebeasi Oct 17, 2023
30425fa
Update menu.tsx
liamdebeasi Oct 17, 2023
9cb3540
Update core/src/components/split-pane/test/multiple/index.html
liamdebeasi Oct 19, 2023
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: 14 additions & 0 deletions core/src/components/menu/menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,20 @@ export class Menu implements ComponentInterface, MenuI {

@Listen('ionSplitPaneVisible', { target: 'body' })
onSplitPaneChanged(ev: CustomEvent) {
const { target } = ev;
const closestSplitPane = this.el.closest('ion-split-pane');

/**
* Menu listens on the body for "ionSplitPaneVisible".
* However, this means the callback will run any time
* a SplitPane changes visibility. As a result, we only want
* Menu's visibility state to update if its parent SplitPane
* changes visibility.
*/
if (target !== closestSplitPane) {
return;
}

this.isPaneVisible = ev.detail.isPane(this.el);
this.updateState();
}
Expand Down
69 changes: 69 additions & 0 deletions core/src/components/split-pane/test/multiple/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Split Pane - Multiple</title>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no"
/>
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" />
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" />
<script src="../../../../../scripts/testing/scripts.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
</head>
<body>
<ion-split-pane id="pane-one" content-id="content-one">
<ion-menu id="pane-one-menu-one" side="start" content-id="content-one">
<div class="ion-padding">Split One Menu Two</div>
liamdebeasi marked this conversation as resolved.
Show resolved Hide resolved
</ion-menu>

<div class="ion-page" id="content-one">
<ion-content class="ion-padding">
Page Content One
<button id="show-pane-two" onclick="showSecondPane()">Show Second Pane</button>
</ion-content>
</div>

<ion-menu id="pane-one-menu-two" side="end" content-id="content-one">
<div class="ion-padding">Split One Menu Two</div>
</ion-menu>
</ion-split-pane>

<ion-split-pane id="pane-two" content-id="content-two" disabled="true" class="ion-page-hidden">
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if this is related to this PR, but there seems to be something strange going on with the menus when trying to open them with gestures. At least one of the four menus can't be opened, and sometimes leads to errors in the console while dragging. It seems inconsistent which one(s); I tried a few more refreshes after the below recording and got different results.

2023-10-19.09-56-33.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's this issue: #19410

The menus all get added to the same document, so there's an inconsistency in the order in which they open because it's dependent on which menus registers first. For example, if you refresh a few times you'll eventually find that the menu you could open before no longer opens. In reality, a menu that is hidden is being activated instead. However, if that menu has display: none then it will have a width of 0, resulting in the animation error (because we're dividing by 0 when computing the gesture step).

<ion-menu id="pane-two-menu-one" side="start" content-id="content-two">
<div class="ion-padding">Split Two Menu One</div>
</ion-menu>

<div class="ion-page" id="content-two">
<ion-content class="ion-padding">
Page Content Two
<button id="show-pane-one" onclick="showFirstPane()">Show First Pane</button>
</ion-content>
</div>

<ion-menu id="pane-two-menu-two" side="end" content-id="content-two">
<div class="ion-padding">Split Two Menu Two</div>
</ion-menu>
</ion-split-pane>

<script>
const splitPaneOne = document.querySelector('ion-split-pane#pane-one');
const splitPaneTwo = document.querySelector('ion-split-pane#pane-two');
const showSecondPane = () => {
splitPaneOne.disabled = true;
splitPaneOne.classList.add('ion-page-hidden');

splitPaneTwo.disabled = false;
splitPaneTwo.classList.remove('ion-page-hidden');
};
const showFirstPane = () => {
splitPaneOne.disabled = false;
splitPaneOne.classList.remove('ion-page-hidden');

splitPaneTwo.disabled = true;
splitPaneTwo.classList.add('ion-page-hidden');
};
</script>
</body>
</html>
38 changes: 38 additions & 0 deletions core/src/components/split-pane/test/multiple/split-pane.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { expect } from '@playwright/test';
import { configs, test, Viewports } from '@utils/test/playwright';

configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => {
test.describe(title('split-pane: multiple'), () => {
test('using multiple split panes should not hide a menu in another split pane', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/18683',
});

await page.setViewportSize(Viewports.large);
await page.goto(`/src/components/split-pane/test/multiple`, config);

const paneOneMenuOne = page.locator('ion-menu#pane-one-menu-one');
const paneOneMenuTwo = page.locator('ion-menu#pane-one-menu-two');

const paneTwoMenuOne = page.locator('ion-menu#pane-two-menu-one');
const paneTwoMenuTwo = page.locator('ion-menu#pane-two-menu-two');

const showPaneOne = page.locator('button#show-pane-one');
const showPaneTwo = page.locator('button#show-pane-two');

await expect(paneOneMenuOne).toBeVisible();
await expect(paneOneMenuTwo).toBeVisible();

await showPaneTwo.click();

await expect(paneTwoMenuOne).toBeVisible();
await expect(paneTwoMenuTwo).toBeVisible();

await showPaneOne.click();

await expect(paneOneMenuOne).toBeVisible();
await expect(paneOneMenuTwo).toBeVisible();
});
});
});
Loading