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: ScrollStackControllerDelegate methods calls #29

Conversation

NicFontana
Copy link
Contributor

This PR adresses an issue that causes calls to the ScrollStackControllerDelegate methods

  • scrollStackRowDidBecomeVisible(_:row:index:state:)
  • scrollStackRowDidBecomeHidden(_:row:index:state:)

before the completion of the ScrollStack layout process, when each rows still don't have a valid frame.

This can happen for example when a ViewController uses a ScrollStack as a subview and inserts rows in a for-loop inside its viewDidLoad() method.

Now these methods are called:

  1. Only once after all the rows have been added and the ScrollStack has been fully laid out
  2. When the user scrolls

A fix has been also made to the dispatchRowsVisibilityChangesTo(_:) ScrollStack method: it now takes into account new inserted rows which still don't have a previous visibility state.


Finally, the call to askForCutomizedSizeOfContentView(animated:) of ScrollStackRow has been moved back in the updateConstraints() method following the best practices described in ModifyingConstraints:

To batch a change, instead of making the change directly, call the setNeedsUpdateConstraints method on the view holding the constraint. Then, override the view’s updateConstraints method to modify the affected constraints.
Always call the superclasses implementation as the last step of your updateConstraints method’s implementation.

This is necessary because we call setNeedsUpdateConstraints() in the layoutUI() method of ScrollStackRow.

…plementation

Also move back `ScrollStackRow` call to `askForCutomizedSizeOfContentView(animated:)` in `updateConstraints()` method.
@malcommac
Copy link
Owner

Hi @NicFontana , thank you for PR. I would also add the new logic to get the size of the view by reading contentView instead of stackView as described in this commit. Would you add it to this PR so we'll avoid to merge both of them.

@NicFontana
Copy link
Contributor Author

I'm not sure I've correctly understood, where should I add this logic?

@malcommac malcommac merged commit da3787b into malcommac:master May 20, 2024
@malcommac
Copy link
Owner

my bad, it was already on develop. merging it, thanks!

@malcommac malcommac self-requested a review May 20, 2024 14:49
@malcommac malcommac added the bug Something isn't working label May 20, 2024
@malcommac malcommac added this to the 1.7.0 milestone May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants