Skip to content

Creister/check ishidden changed#27

Merged
marlimox merged 2 commits intomarlimox:masterfrom
creister:creister/check-ishidden-changed
Dec 17, 2018
Merged

Creister/check ishidden changed#27
marlimox merged 2 commits intomarlimox:masterfrom
creister:creister/check-ishidden-changed

Conversation

@creister
Copy link
Copy Markdown
Contributor

@creister creister commented Dec 14, 2018

Hi, we found that multiple calls to hideRow (with true) can cause unhiding to fail, this is due to isHidden having to be called inside the animation block. This PR simply checks that isHidden is changing to avoid multiple calls causing issues.

Currently this test will fail, as hiding row with animation multiple
times syncronously will cause unhiding to fail
Copy link
Copy Markdown
Owner

@marlimox marlimox left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you for fixing this @creister!

/// If `animated` is `true`, the change is animated.
open func setRowHidden(_ row: UIView, isHidden: Bool, animated: Bool = false) {
guard let cell = row.superview as? StackViewCell else { return }
guard cell.isHidden != isHidden else { return }
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Style: put this extra condition in the first guard statement after a comma.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed 👍

This fixes failing test testSetRowHiddenDuplicateCalls
@creister creister force-pushed the creister/check-ishidden-changed branch from 8a66282 to 94668d6 Compare December 17, 2018 15:02
@marlimox marlimox merged commit 846b7ff into marlimox:master Dec 17, 2018
@creister creister deleted the creister/check-ishidden-changed branch January 31, 2019 19:02
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