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.

Sign up for GitHub

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

Jump to bottom

Remove "Free Shipping" Banner #800

Merged
merged 10 commits into from
Apr 19, 2022
Merged

Remove "Free Shipping" Banner #800

merged 10 commits into from
Apr 19, 2022

Conversation

charlieyu1996
Copy link
Contributor

Background

This PR reduces unnecessary code for displaying the free shipping banner. Since it does not add value to the demo.

Fixes

Change Summary

  • Removed shipping fee banner in header.html
  • Removed related CSS in styles.css
  • Removed logic in main.go GetQuote where it counts the number items for the quote
  • Removed logic in quote.go quoteByCountFloat that checks for item count to be 0

Additional Notes

@charlieyu1996 charlieyu1996 requested a review from a team as a code owner April 14, 2022 17:45
@github-actions
Copy link

🚲 PR staged at http://35.193.111.39

@charlieyu1996 charlieyu1996 reopened this Apr 14, 2022
@charlieyu1996 charlieyu1996 reopened this Apr 14, 2022
@github-actions
Copy link

🚲 PR staged at http://35.193.111.39

@xtineskim
Copy link
Contributor

closed and re-opened since there was an error with the cartservice building. wanted to retrigger the CI

Shabirmean
Shabirmean reviewed Apr 14, 2022
View reviewed changes
Copy link
Member

@Shabirmean Shabirmean left a comment

Choose a reason for hiding this comment

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

Thank you for addressing this issue @charlieyu1996
Left some minor comments!

src/frontend/templates/header.html Show resolved Hide resolved
src/shippingservice/main.go Outdated Show resolved Hide resolved
src/shippingservice/quote.go Outdated Show resolved Hide resolved
@github-actions
Copy link

🚲 PR staged at http://35.193.111.39

@NimJay NimJay changed the title Shipping fee banner removal Remove "Free Shipping" Banner Apr 14, 2022
NimJay
NimJay approved these changes Apr 14, 2022
View reviewed changes
Copy link
Collaborator

@NimJay NimJay left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks again, @charlieyu1996, for doing this!
And thanks for addressing all the issues Shabir raised.
Everything looks amazing!

And thank you, @Shabirmean, for suggesting the improvements (i.e., realizing we could remove quoteByCountFloat and the mis-numbered comments).

I have done a quick smoke test of the staging URL. Works! 👍

Approved.
But I'll wait until Shabir provides his approval before merging.

@Shabirmean
Copy link
Member

Thank you for the changes @charlieyu1996! LGTM!

@charlieyu1996 charlieyu1996 merged commit 6bbe8cc into main Apr 19, 2022
@charlieyu1996 charlieyu1996 deleted the charlie-shipping-fee-branch branch April 19, 2022 15:24
@NimJay NimJay mentioned this pull request May 17, 2023
Closed
@tsmithv11 tsmithv11 mentioned this pull request Jul 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NimJay NimJay NimJay approved these changes

@Shabirmean Shabirmean Shabirmean approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@charlieyu1996 @xtineskim @Shabirmean @NimJay