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

[MBL-1783 Unhide rewards immediately if there's no shippable rewards #2178

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

ifosli
Copy link
Contributor

@ifosli ifosli commented Oct 16, 2024

πŸ“² What

We currently hide all rewards until the shipping location selector is done loading. If there aren't any shippable rewards, unhide the rewards immediately.

πŸ‘€ See

Jira

βœ… Acceptance criteria

  • Rewards without any shippable rewards show their rewards correctly

guard let self else { return }

self.collectionView.isHidden = !shimmerLoadingViewIsHidden
self.collectionView.layoutIfNeeded()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted this because now that we're not always waiting for the location view to load, it gets called before the view hierarchy is ready. I did test a bunch of random projects (hence the local pickup bug I found) and didn't see any UI weirdness caused by removing this line.

@ifosli ifosli self-assigned this Oct 16, 2024
@ifosli ifosli marked this pull request as ready for review October 16, 2024 22:14
Copy link
Contributor

@stevestreza-ksr stevestreza-ksr left a comment

Choose a reason for hiding this comment

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

Can't really be sure that this behavior is fully correct as I haven't been in this area much, but changes seem to make sense to me!

guard let self else { return }

self.collectionView.isHidden = !shimmerLoadingViewIsHidden
self.collectionView.layoutIfNeeded()
self.collectionView.isHidden = rewardsCollectionViewIsHidden
Copy link
Contributor

Choose a reason for hiding this comment

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

Making sure the intent was to invert this isHidden check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I found the naming to be super confusing before, so I've inverted it (both here and in the view model) to be more intuitive. It's still in sync with the underlying value; it's just the output signal value that's inverted. Thanks for checking though!

@ifosli ifosli merged commit ce18b80 into main Oct 17, 2024
4 checks passed
@ifosli ifosli deleted the showRewardsWithoutShippingLocation branch October 17, 2024 04:54
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.

3 participants