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

Don't animate when dequeuing cells #517

Merged
merged 8 commits into from
Aug 8, 2024
Merged

Don't animate when dequeuing cells #517

merged 8 commits into from
Aug 8, 2024

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented Dec 8, 2023

EDIT: Reverted here #542

https://block.atlassian.net/browse/UI-5081

We now force a non-animated layout before dequeuing a cell, to ensure that any implicit animations that may occur are avoided. This avoids issues where you may see an animation as a reused cell's changed state is applied.

Checklist

Please do the following before merging:

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

@kyleve kyleve changed the title [WIP DNR] Don't animate when dequeuing cells Don't animate when dequeuing cells Jan 10, 2024
@kyleve kyleve marked this pull request as ready for review January 10, 2024 20:49
@kyleve kyleve requested a review from a team January 10, 2024 21:58
Copy link
Collaborator

@n8chur n8chur left a comment

Choose a reason for hiding this comment

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

Seems reasonable, but this won't cause an extra layout, would it? it's just going to happen earlier and non-animated since the cell should be needsLayout = false after this?

@kyleve
Copy link
Collaborator Author

kyleve commented Jan 11, 2024

Seems reasonable, but this won't cause an extra layout, would it? it's just going to happen earlier and non-animated since the cell should be needsLayout = false after this?

I logged calls to layout and the number of calls seemed identical from what I could tell.

* main:
  fix: don't cancel touches in view for tap gesture recognizer
@kyleve
Copy link
Collaborator Author

kyleve commented Jan 11, 2024

This is for some reason breaking the KIF tests, seemingly related to market search bar, will need to dig into that before merging: https://square-console.sqprod.co/bbr/production/ci_jobs/PR+ios+WK-SK4+Bazel-Pipeline-PR/3

* origin/main:
  Prepare 14.3.0 (#540)
  Custom Update Animations (#539)
  Update version to 14.2.0
  Prepare 14.2.0 release, which contains a Blueprint update
  Bumping versions to 14.1.0 (#535)
  reorder control now proxies accessibility into a seperate element (#533)
  Prepare 14.0.3
  Fix a crash that could occur during cell reuse if a list contained different types of headers. The wrong ObjectIdentifier was being compared and stored.
  Get a repro for the reordering crash reported in #market-ios
  chore: Generated documentation now uses a static copyright notice to avoid noisy diffs (#530)
  Bumping versions to 15.0.2
  fix: Fix tap gesture swallowing touches in swipe actions view
  Bump to 14.0.1
  Fix SPM Blueprint dependency
  Release 14.0.0, update BlueprintUI to 3.0.0 (#525)
  chore: iOS 15 deployment target bump [UI-5187] (#524)
  chore: Bump CI to Xcode 15.1. Bump gems. [UI-5186] (#523)
  fix: don't cancel touches in view for tap gesture recognizer
  Revert weak change
  Revert "Revert "Supplementary Tracking Fixes (#433)""
Copy link
Collaborator Author

@kyleve kyleve Aug 1, 2024

Choose a reason for hiding this comment

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

  • A possible bug I found, should fix before merging: ListView.VisibleContent.update should assign
            self.items = newItems
            self.headerFooters = newHeaderFooters

Before callbacks, otherwise a re-rentrant call could call these twice (or more).

@kyleve kyleve merged commit f4e29a7 into main Aug 8, 2024
4 checks passed
@kyleve kyleve deleted the kve/dequeue-animations-2 branch August 8, 2024 22:19
@kyleve kyleve mentioned this pull request Aug 10, 2024
1 task
kyleve added a commit that referenced this pull request Aug 12, 2024
This reverts out the fixes from
#517, since it breaks our KIF
tests in POS. I missed this previously due to test selection. I've also
added a testing VC to iterate on this more.
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.

2 participants