fix: helper function that determines the ion-menu animation side now …#31214
fix: helper function that determines the ion-menu animation side now …#31214Austin-Spriggs wants to merge 1 commit into
Conversation
…takes parent element direction attribute into consideration
|
@Austin-Spriggs is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Hey, thanks for the PR! Sorry we somehow overlooked this one. In addition to the failing tests (which I think are lint failures mostly, probably - just run lint.fix in core), I noticed a few other issues
| * If the document direction changes, we need to create a new animation. | ||
| */ | ||
| const isEndSide = isEnd(this.side); | ||
| const isEndSide = isEnd(this.side, this.el); |
There was a problem hiding this comment.
I think sideChanged() needs the same treatment. menu.tsx:153 still calls isEnd(this.side) with no host element, so it resolves from document.dir, while this line now resolves from the element's dir. In the ancestor-dir="rtl" case those two disagree, and this.isEndSide feeds the gesture math (checkEdgeSide, computeDelta) and the animation builders. Can you pass this.el at line 153 too so both call sites agree?
| */ | ||
| export const isEndSide = (side: Side): boolean => { | ||
| const isRTL = document.dir === 'rtl'; | ||
| export const isEndSide = (side: Side, hostElement?: Element): boolean => { |
There was a problem hiding this comment.
Could we get a test for the ancestor-dir="rtl" case? contributing.md asks for a test on behavior changes, and this one is easy to regress. A spec that sets dir="rtl" on a wrapper and asserts the animation side would lock it in.
| const isRTL = hostElement | ||
| ? hostElement.closest('[dir]')?.getAttribute('dir') === 'rtl' | ||
| : document.dir === 'rtl' | ||
| ; |
There was a problem hiding this comment.
| const isRTL = hostElement | |
| ? hostElement.closest('[dir]')?.getAttribute('dir') === 'rtl' | |
| : document.dir === 'rtl' | |
| ; | |
| const isRTL = hostElement | |
| ? hostElement.closest('[dir]')?.getAttribute('dir') === 'rtl' | |
| : document.dir === 'rtl'; |
This is indented with tabs and has the ; hanging on its own line, so it fails npm run lint. npm run lint.fix would also sort it.
| * based on the value of dir | ||
| * @param side the side | ||
| * @param hostElement the host element | ||
| * @param isRTL whether the application dir is rtl |
There was a problem hiding this comment.
The @param isRTL line is stale, there's no isRTL parameter. Since you're adding @param hostElement here anyway, mind dropping the isRTL line too?
2 participants
fix: helper function that determines the ion-menu animation side now takes parent element direction attribute into consideration
Issue number: resolves #30226
What is the current behavior?
Hamburger menu is not animating from right to left when within an ion-app component with attribute dir="rtl".
What is the new behavior?
I modified the helper function that determines which side the animation is played from so that the direction attribute of the menu's parent elements can be determined before setting the animation direction.
Does this introduce a breaking change?