Skip to content

Conversation

@rpeel-sq
Copy link
Contributor

@rpeel-sq rpeel-sq commented Jul 29, 2025

Reverts #542, to fix unexpected cell animation bug.

Demo

Before

before.mov

After

after.mov

Checklist

Please do the following before merging:

  • Ensure any public-facing changes are reflected in the changelog. Include them in the Main section.

@rpeel-sq rpeel-sq force-pushed the rpeel/rst-27420-test-reverting-the-revert branch from 352623e to d85f305 Compare July 29, 2025 02:43
@johnnewman-square
Copy link
Contributor

Based on our discussion in Slack, I think we can make this tweak official! It would be great if we had a demo that showcased the fixed behavior, where the call to:

UIView.performWithoutAnimation {
    cell.layoutIfNeeded()
}

showed animations no longer running.

@rpeel-sq rpeel-sq force-pushed the rpeel/rst-27420-test-reverting-the-revert branch from d85f305 to 5ba48eb Compare August 25, 2025 17:44
@rpeel-sq rpeel-sq changed the title [Do not merge] Revert "Merge pull request #512 from square/kve/remove-override" [RST-278240] Reverts #512 Aug 25, 2025
@rpeel-sq rpeel-sq changed the title [RST-278240] Reverts #512 [RST-278240] Reverts https://github.com/square/Listable/pull/512 to fix unexpected cell animation bug Aug 25, 2025
@rpeel-sq rpeel-sq changed the title [RST-278240] Reverts https://github.com/square/Listable/pull/512 to fix unexpected cell animation bug [RST-27240] Reverts https://github.com/square/Listable/pull/512 to fix unexpected cell animation bug Aug 25, 2025
@rpeel-sq rpeel-sq changed the title [RST-27240] Reverts https://github.com/square/Listable/pull/512 to fix unexpected cell animation bug [RST-27240] Reverts 506307c58a4f2823fa6bebb10371d7be5723f3f9 to fix unexpected cell animation bug Aug 25, 2025
@rpeel-sq rpeel-sq changed the title [RST-27240] Reverts 506307c58a4f2823fa6bebb10371d7be5723f3f9 to fix unexpected cell animation bug [RST-27240] Reverts https://github.com/square/Listable/pull/542 to fix unexpected cell animation bug Aug 25, 2025
@rpeel-sq rpeel-sq changed the title [RST-27240] Reverts https://github.com/square/Listable/pull/542 to fix unexpected cell animation bug [RST-27240] Fixes unexpected cell animation bug Aug 25, 2025
@rpeel-sq rpeel-sq marked this pull request as ready for review August 25, 2025 17:57
@rpeel-sq rpeel-sq force-pushed the rpeel/rst-27420-test-reverting-the-revert branch 2 times, most recently from 6970a4e to a7d7eac Compare August 25, 2025 19:32
@rpeel-sq rpeel-sq force-pushed the rpeel/rst-27420-test-reverting-the-revert branch from 42a1d01 to 201a59c Compare August 25, 2025 19:39
@rpeel-sq rpeel-sq force-pushed the rpeel/rst-27420-test-reverting-the-revert branch from eb35e62 to 7a9b0cc Compare August 25, 2025 20:24
Copy link
Contributor

@johnnewman-square johnnewman-square 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 good! I'm going to ask the team for one additional review around the github workflow changes and whether or not we need aria2.

@johnnewman-square johnnewman-square requested a review from a team August 25, 2025 21:18
Comment on lines +49 to +50
- name: Install aria2
run: brew install aria2
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to ask the team for one additional review around the github workflow changes and whether or not we need aria2.

I think aria2 will still be used implicitly in the following step so probably worth keeping around. You might be able to find some hints buried in the build logs but I guess the best way to verify would be to spin up a PR without it and compare the total CI times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @robmaceachern! This draft PR removed aria2 and CI took 5m 51s. That seems to match previous results, but I'm on board with keeping it around if we'd like to minimize the changes here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it looks like it's either not being used or makes no real difference. Probably fine to remove!

Copy link
Member

Choose a reason for hiding this comment

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

Oh actually I just noticed this:
image

So it might be that just removing the install step doesn't necessarily mean that aria2 won't already be installed on the machine. Not sure if github includes it or the runner was cached or something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I remove or leave 😅?

Copy link
Member

Choose a reason for hiding this comment

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

Haha just leave it 👍🏽

@johnnewman-square johnnewman-square merged commit a69ed0c into square:main Aug 25, 2025
3 checks passed
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.

3 participants