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

feat(rtl): menu update side in runtime #11336

Closed
wants to merge 79 commits into from

Conversation

AmitMY
Copy link
Contributor

@AmitMY AmitMY commented Apr 23, 2017

WIP

Short description of what this resolves:

Update menu type when platform direction changes

Changes proposed in this pull request:

  • Move the menu-type functionality from constructor to a separate method
  • Store the correct side in _side, don't save normalised side
  • isRightSide is now a magic method instead of a variable (avoid mistakes when changing dir in runtime)
  • Add event emitter to when direction changes, and update the type when it does

Explanation:

  • First thing is to add support for menuToggle for start and end (just missing from menu in general)
  • In menu types, I moved a part of the constructor to another method updatePosition, which sets the menu enter / close animation. (This worked when you start ltr, or start rtl, but not if you want to switch)
  • I added an event emitter to platform, to know when direction changes, and when it does, I call the updatePosition again to set the correct animation (because start/end are now the opposite sides)
  • I also changed the basic test to use start/end instead of left and right.

Problem:

If you switch direction, the updatePosition is called again, and instead of overriding the animation, it seems to clash with it, so it animates from the middle, see: https://youtu.be/3VFDO7otYrU

Ionic Version: 3.x

All tests passing

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 23, 2017

@manucorporat This one completes #11233
Let me know what you think

@AmitMY
Copy link
Contributor Author

AmitMY commented May 3, 2017

@manucorporat ?

@AmitMY AmitMY mentioned this pull request Jun 13, 2017
11 tasks
update app-scripts to 1.3.11, add npm 5 support
…n the short term

support for named ion-nav/ion-tabs to improve url in the short term
…'_transitionFinish', provide no

mark as not transitioning on success in addition to '_transitionFinish', provide note as to why we
want it in both places
@chukwu
Copy link

chukwu commented Jul 5, 2017

Based on the youtube video linked from @AmitMY, this solves my problem.
When is this going to be shipped? What version is this expected?

@chukwu
Copy link

chukwu commented Jul 5, 2017

Everything explained by @AmitMY was what I expected.

  1. An observable/Event Emitter which listens to setDir and updates dom especially when updatedocument argument is set to true on runtime.
  2. Dynamically toggle menu sides attributes from right to left and vice-versa.

Guys, I am working on multiple production projects which strongly requires this to work on runtime without manually reloading the page and managing states.

@AmitMY
Copy link
Contributor Author

AmitMY commented Jul 5, 2017

@chukwu Same as you, I am looking forward this getting merged. Unfortunetly, there is a bug here, because I don't clear the animation state before re-setting it. (so it partially works on android/windows, and does not on iOS)
I am in contact with some Ionic members to help fix that issue, and if/when that is fixed, it can be merged. However, currently there is kind of a code freeze on v3, and that is why so many PRs (~25) are not being reviewed.

@mahmoudrabie
Copy link

We need help from IONIC team to solve this problem ASAP.

@Ionitron
Copy link
Collaborator

Hello and thank you for contributing to Ionic! We have been working on porting all of the Ionic components to web components and have recently updated master to reflect this. This significant change has caused this pull request to break. While we really appreciate the time and effort you put into creating this, we are not able to merge it because of the newly introduced conflicts. We are extremely sorry about this. We will not be merging any more features in to v3. If this is a feature and you have the time, please resubmit this PR against the master branch. If this is a critical security issue in v3, we would greatly appreciate it if you would resubmit the PR against the new v3 branch. Thanks so much for your time!

@Ionitron Ionitron closed this Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.