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(menu): emit ionMenuChange when re-mounted #28049

Merged
merged 7 commits into from
Aug 24, 2023
Merged

fix(menu): emit ionMenuChange when re-mounted #28049

merged 7 commits into from
Aug 24, 2023

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Aug 23, 2023

Issue number: resolves #28030


What is the current behavior?

ion-menu registers itself with the menu controller when connectedCallback fires and then unregisters itself when disconnectedCallback fires. When the menu was removed from the DOM, disconnectedCallback was not always being fired due to ionic-team/stencil#4070.

ion-menu-button checks to see if it should be visible by grabbing the current menu in

this.visible = await updateVisibility(this.menu);
.

Since disconnectedCallback was not being fired, ion-menu-button would still find the menu even when it was no longer in the DOM. In this case, the menu was not being unregistered due to disconnectedCallback not firing.

When the linked Stencil bug was resolved in Stencil 4.0.3, the menu button started to disappear. This is happening because disconnectedCallback is now correctly called when the menu is removed from the DOM. Since disconnectedCallback is called, the menu is un-registered from the menu controller, and the menu button can no longer find it.

However, this revealed a long-standing bug where re-adding the menu would not fire ionMenuChange again. As a result, the menu button remained hidden.

What is the new behavior?

  • The menu now fires ionMenuChange on connectedCallback as long as componentDidLoad has already been run.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 7.3.2-dev.11692803611.15c1bc87

@liamdebeasi liamdebeasi changed the title Fw 5015 fix(menu): emit ionMenuChange when re-mounted Aug 23, 2023
@github-actions github-actions bot added the package: core @ionic/core package label Aug 23, 2023
@stackblitz
Copy link

stackblitz bot commented Aug 23, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@liamdebeasi liamdebeasi marked this pull request as ready for review August 23, 2023 15:40
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

The most minor nit, implementation looks good.

core/src/components/menu-button/test/async/index.html Outdated Show resolved Hide resolved
</ion-header>
<ion-content class="ion-padding">
Main Content
<button onclick="trigger()" id="trigger">Add Menu To DOM</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

If I click the button multiple times, the menu is still present in the DOM but the menu button disappears; is that what's supposed to happen? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like connectedCallback fires then disconnectedCallback, so it could be a Stencil bug. I was able to reproduce this behavior even after I removed the fix.

@liamdebeasi liamdebeasi added this pull request to the merge queue Aug 24, 2023
Merged via the queue into main with commit fa41131 Aug 24, 2023
@liamdebeasi liamdebeasi deleted the FW-5015 branch August 24, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: menu button cannot find menu if menu is moved in DOM
3 participants