Skip to content

Conversation

@thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Mar 31, 2023

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

  • When the document direction is dynamically changed, the menu does not switch to the correct position. For example, <ion-menu side='start'> will always be on the left side regardless of direction.
  • When the side property is dynamically changed, the animation starts in the middle of the screen instead of off screen.

Issue URL: resolves #25601 #19489

What is the new behavior?

  • Animation is smooth regardless of side property or document direction being dynamically change.
  • Menu appears on the correct side based on document direction when dynamically updated.

Does this introduce a breaking change?

  • Yes
  • No

Other information

N/A

@thetaPC thetaPC requested review from a team and averyjohnston as code owners March 31, 2023 20:56
@bolt-new-by-stackblitz
Copy link

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

@github-actions github-actions bot added the package: core @ionic/core package label Mar 31, 2023
@averyjohnston
Copy link
Contributor

The newly added test is failing because it includes new screenshot checks. Make sure you run the Update Reference Screenshots job for this branch 👀 (I often like to leave my PRs in draft until all tests are passing, so it's more clear that it's actually ready for review.)

@thetaPC thetaPC marked this pull request as draft April 3, 2023 16:13
@thetaPC thetaPC marked this pull request as ready for review April 3, 2023 17:35
@thetaPC
Copy link
Contributor Author

thetaPC commented Apr 3, 2023

@amandaejohnston I've updated the references and the PR is ready to go.

Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

This looks great! Just had a small request on the tests.

@thetaPC thetaPC requested a review from averyjohnston April 4, 2023 17:38
Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

Nice work!

@thetaPC thetaPC added this pull request to the merge queue Apr 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 4, 2023
@liamdebeasi liamdebeasi added this pull request to the merge queue Apr 4, 2023
Merged via the queue into main with commit a35886e Apr 4, 2023
@liamdebeasi liamdebeasi deleted the FW-1852 branch April 4, 2023 19:25
@thetaPC thetaPC mentioned this pull request Apr 7, 2023
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: dynamically changing side prop for menu not working

5 participants