feat(menu-toggle): migrate to Modular Ionic#31171
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
package: core
| try { | ||
| this.visible = await updateVisibility(this.menu); | ||
| } catch { | ||
| this.visible = false; | ||
| } |
There was a problem hiding this comment.
connectedCallback and the @Listen handlers discard the Promise returned by visibilityChanged(), so any rejection would surface as unhandled. The current chain can't reject, but catching here makes the method safe against future changes to menuController.get() (from @utils/menu-controller) or ion-menu's isActive.
github.com:ionic-team/ionic-framework into FW-6854 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Looks great! I have a couple of nits but feel free to disregard
| try { | ||
| this.visible = await updateVisibility(this.menu); | ||
| } catch { | ||
| this.visible = false; | ||
| } |
There was a problem hiding this comment.
Nit: This try/catch is a nice change, but do we want a test covering that path? The basic e2e only exercises open/close right now, so nothing locks in the rejection fallback.
| The following breaking changes apply to `ion-menu-toggle`: | ||
|
|
||
| 1. Theme classes (`ion-menu-toggle.md`, `ion-menu-toggle.ios`) are no longer supported. | ||
|
|
||
| <h5>Theme classes</h5> | ||
|
|
||
| Remove any instances that target the theme classes: `ion-menu-toggle.md`, `ion-menu-toggle.ios`. |
There was a problem hiding this comment.
Nit: This reads a little inconsistent with the other single-change entries. Item Divider and Spinner both state the theme-class removal as one bullet, no numbered list or <h5>. The numbered-list-plus-<h5> shape is what Content uses, but Content has four separate changes; here item 1 and the <h5>Theme classes</h5> body say nearly the same thing. Maybe we should collapse it to a single bullet to match item-divider? Totally up to you, probably doesn't matter much
3 participants
Issue number: resolves internal
What is the current behavior?
ion-menu-toggledoes not fragment styles based on themes. All 3 themes share one style. However, it's not configured to the Modular Ionic.What is the new behavior?
themeprop has been removed. Theiosandmdclasses are no longer applied to theion-menu-toggleelement.Does this introduce a breaking change?
This PR introduces breaking changes to how
ion-menu-toggleis styled.Migration Path:
ion-menu-toggle.md,ion-menu-toggle.ios.Other information
Previews