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-1605] Display Estimated Shipping on Rewards and Add-Ons #2117

Merged
merged 14 commits into from
Aug 20, 2024

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Aug 12, 2024

📲 What

I recommend reviewing this commit by commit

Displays estimated shipping range, if there are any set, on reward card and add-on card views.

🤔 Why

See jira ticket

🛠 How

Updates RewardCardView and RewardAddOnView
Uses the same styling as the other sections of the cards.

ShippingRule structs contain the newly exposed estimatedMin and estimatedMax values that are set in the project config.
Since a reward can have multiple ShippingRules, we need to pass the ShippingRule that is selected in the location dropdown to the RewardCardView so that we can determine which ShippingRule info to display.

This only applies to rewards cards because we already pass in the relevant ShippingRule to the AddOns screen.

From there it's just getting the new UI elements in place and making sure this is only shown when the feature flag is on and when there is actually estimated shipping values to show.

  • Estimated shipping isn't required by creators so if there is none, we don't show anything.

👀 See

Simulator Screen Recording - iPhone 15 Pro - 2024-08-12 at 14 05 21

✅ Acceptance criteria

  • Estimated shipping is shown on reward cards and addon cards when feature flag is on an values are set

⏰ TODO

  • We'll get the hardcoded strings translated and updated later before release.

@scottkicks scottkicks force-pushed the scott/mbl-1605/display-estimatedshipping branch from 002e326 to c55cc60 Compare August 12, 2024 21:34
@scottkicks scottkicks marked this pull request as ready for review August 14, 2024 18:23
@scottkicks
Copy link
Contributor Author

FYI this has some extra commits that will be on main by now. Formatting that Ingerid fixed + exposing estimated shipping properties stuff that I did. These should be cleaned up once main is up to date.

@scottkicks scottkicks requested a review from ifosli August 14, 2024 18:24
Copy link
Contributor

@ifosli ifosli left a comment

Choose a reason for hiding this comment

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

Looks pretty good but please make sure you handle the "estimated min and max are equal" case and add snapshot tests for this before submitting. Marking this as approved because I don't think I need to re-review it, but please lmk if you want me to take a look at any changes!

_ = self.estimatedShippingStackView
|> includedItemsStackViewStyle

// TODO: Update string with translations
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Might be helpful to tag a ticket number here to make it easier to find all the strings once translations are ready

let formattedMax = String(format: "%.0f", estimatedMax)

// TODO: Update string with translations
return "About $\(formattedMin)-$\(formattedMax)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to check if formattedMin == formattedMax and just use ex About $\(formattedMin)$ if they're equal. Will you fix and maybe add tests for this function specifically, especially if it's not already covered by other tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add snapshot tests to this where the estimated shipping shows? I think one where the min and max are different and a different one where they're equal should cover it, but feel free to cover other edge cases if I'm missing something!

@scottkicks scottkicks merged commit ce806b5 into main Aug 20, 2024
5 checks passed
@scottkicks scottkicks deleted the scott/mbl-1605/display-estimatedshipping branch August 20, 2024 01:52
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.

2 participants