-
Notifications
You must be signed in to change notification settings - Fork 13.5k
fix(menu): menu no longer disappears with multiple split panes #28370
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
Merged
Merged
Changes from all 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 2ef1f1e
add failing test
liamdebeasi 30ecadb
fix test
liamdebeasi d4377c2
add more menus
liamdebeasi ac87d08
fix(menu): menu no longer disappears with multiple split panes
liamdebeasi 30425fa
Update menu.tsx
liamdebeasi 9cb3540
Update core/src/components/split-pane/test/multiple/index.html
liamdebeasi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 One</div> | ||
</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"> | ||
<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
38
core/src/components/split-pane/test/multiple/split-pane.e2e.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).