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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect Reorder.Item order calculation (Part 2) #2359

Merged
merged 3 commits into from
Nov 29, 2023
Merged

Fix incorrect Reorder.Item order calculation (Part 2) #2359

merged 3 commits into from
Nov 29, 2023

Conversation

chuganzy
Copy link
Contributor

@chuganzy chuganzy commented Oct 6, 2023 •
edited

This PR aims to complete @obeattie's PR here by adding two more commits:

  1. cae864c Get rid of a null check that should be no longer necessary
  2. ce65afb Add a cypress test that is mostly reproducible.

Note:

Regarding the cypress test, unfortunately to reproduce this on ARM64 macOS (I am on M2), I needed to upgrade cypress to the latest to run the test on ARM64 native browsers - otherwise test passes on the main because of the poor browser performance on Rosetta 2, however I did not include that change here to keep this PR minimum.

@chuganzy chuganzy mentioned this pull request Oct 6, 2023
Closed
@chuganzy chuganzy changed the title Fix incorrect Reorder.Item order calculation Fix incorrect Reorder.Item order calculation (Part 2) Oct 7, 2023
@melrose13-69
Copy link

Thank you for the work done, what is the status, these changes would help me in moment

@ponbac
Copy link

ponbac commented Oct 13, 2023

Getting this merged would be great!

@nick-keller
Copy link

Awesome work, thanks!

@nick-keller
Copy link

Hey @mattgperry, do you need anything to merge this PR?
I completely understand how tough it must be to manage such a project. It a simple fix to a major feature of the lib so let us know how we can help!

@Swiftwork
Copy link

Wanted to lend my support to this as well. Ran into both the issue this PR resolves and @chuganzy other PR #2264. Both are blockers for us to update Framer Motion to a later version and to use the otherwise excellent Reorder components. Let's get this merged and released unless there is another blocker.

@chuganzy
Copy link
Contributor Author

Rebased the branch on main since there was a conflict in yarn.lock.

@mattgperry
Copy link
Collaborator

Thanks for the test and apologises for the delay - busy on Page Effects. Let me merge and publish now.

@mattgperry mattgperry added the automerge Land this PR label Nov 29, 2023
@mattgperry mattgperry merged commit 4bbdc3f into framer:main Nov 29, 2023
1 check passed
@chuganzy chuganzy deleted the reoder-fix branch November 29, 2023 14:30
@chuganzy
Copy link
Contributor Author

@mattgperry Thank you for taking a look while you have a lot on your plate!

There is a separated PR to eliminate the Reorder's freezing issue here. It would be great if you had a chance to take a look on it. Thanks!

decahedron1 pushed a commit to vitri-ent/motion that referenced this pull request Dec 5, 2023
@chuganzy @obeattie @decahedron1
Fix incorrect Reorder.Item order calculation (Part 2) (framer#2359)
6de2507 
* Don't rely on effect execution order when passing around the layout of Reorder.Items

* Get rid of layout check as it is no longer nullable

* Add an integration test

---------

Co-authored-by: Oliver Beattie <oliver@obeattie.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@chuganzy @melrose13-69 @ponbac @nick-keller @Swiftwork @mattgperry @obeattie