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

Fix #1275 layouts inconsistency in updateAnimated:completion of IGListBindingSectionController #1285

Conversation

qhhonx
Copy link
Contributor

@qhhonx qhhonx commented Nov 29, 2018

Changes in this pull request

Issue fixed: #1275

This PR is straintforward solution as mentioned in #1275. When I finished it, I realized that it is not a problem in IGListCollectionViewLayout Partial Optimization and IGListBindingSectionController deserved it. Maybe IGListCollectionViewLayout is rarely used because of great builtin UICollectionViewFlowLayout or cells in section rarely need changing their sizes in practices.

Despite it is not IGListCollectionViewLayout's fault, I think it can be more optimized against invalidateLayoutWithContext. I would like to make an another PR to optimize it.

Due to this PR just giving a proposed solution, it lacks of adding tests and changing log. When this solution is accepted, I would like to complete these todos.

Checklist

  • All tests pass. Demo project builds and runs.
  • I added tests, an experiment, or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have reviewed the contributing guide

…del but not invalidating their layout attributes for out-of-box ListBindingSectionController
@iglistkit-bot
Copy link

iglistkit-bot commented Nov 29, 2018

2 Warnings
⚠️ All pull requests should have a milestone attached, unless marked #trivial.
⚠️ Adding or removing library source files requires updating the examples. Please run ./scripts/pod_setup.sh from the root directory and commit the changes.

Generated by 🚫 Danger

Tests/IGListBindingSectionControllerTests.m Show resolved Hide resolved
Source/IGListBindingSectionController.m Outdated Show resolved Hide resolved
Source/IGListAdapter.m Outdated Show resolved Hide resolved
Source/IGListStackedSectionController.m Outdated Show resolved Hide resolved
@qhhonx
Copy link
Contributor Author

qhhonx commented Jan 8, 2019

Hi, @rnystrom , I have added a experiments bitmask to enable the changes, please review again.

@qhhonx
Copy link
Contributor Author

qhhonx commented May 25, 2019

@lorixx It seems that you are active maintainer now. Can you review this PR and import it?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@candance has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@qhhonx
Copy link
Contributor Author

qhhonx commented Jun 11, 2019

Can this PR ready to be merged or any else problem exists?

Copy link
Contributor

@lorixx lorixx left a comment

Choose a reason for hiding this comment

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

Looks good overall, the only main feedback I have is around that IGTestDiffingSectionController is doing too much thing and should not be a one-for-all kind of class, would you mind creating a different SectionController and DataSource for the IGLayoutTestItem dependent use case?

Tests/Objects/IGTestDiffingSectionController.m Outdated Show resolved Hide resolved
Tests/IGListBindingSectionControllerTests.m Outdated Show resolved Hide resolved
Tests/IGListBindingSectionControllerTests.m Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@qhhonx has updated the pull request. Re-import the pull request

@qhhonx qhhonx force-pushed the fix-BindSectionController-updates-not-invalidating-layouts branch from 03f1704 to f775b3f Compare June 12, 2019 03:16
@facebook-github-bot
Copy link
Contributor

@qhhonx has updated the pull request. Re-import the pull request

@qhhonx
Copy link
Contributor Author

qhhonx commented Jun 12, 2019

Looks good overall, the only main feedback I have is around that IGTestDiffingSectionController is doing too much thing and should not be a one-for-all kind of class, would you mind creating a different SectionController and DataSource for the IGLayoutTestItem dependent use case?

@lorixx I have accepted your suggestion and implemented it. Please review it again and give some comments if any.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@lorixx lorixx left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks Qinghua!

@facebook-github-bot
Copy link
Contributor

@lorixx merged this pull request in 08bf69c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IGListCollectionViewLayout partial optimization issue in certain situation
5 participants