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

chore(styles): make subscription icon background configurable as metadata #5973

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

johngruen
Copy link
Contributor

Because

Icons for mozilla services will be white and will need different bg colors

This pull request

makes bg color configurable as stripe metadata. Any valid CSS color value can be included in metadata (gradients, color names etc)

Issue that this pull request solves

Closes: #5952

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).

Notes:

  • I may need to add a tests here, but could use a bit of help/direction if so
  • I will add a PR for ecosystem docs to highlight the new metadata key once this lands

Screenshots (Optional)

image
image

@@ -46,6 +46,7 @@ export const SELECTED_PLAN: Plan = {
interval_count: 1,
product_metadata: {
webIconURL: 'http://placekitten.com/49/49?image=2',
webIconBackground: 'purple',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i pretty much added a color as mock data anywhere an icon was supplied

@johngruen
Copy link
Contributor Author

There are tests for not including an icon in metadata (for example here)...i may need to add these, but could use a little help if so...

@johngruen johngruen force-pushed the make-product-icon-bg-metadata branch from a8cb7f1 to ab231fa Compare July 17, 2020 13:50
@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #5973 into main will decrease coverage by 1.03%.
The diff coverage is 88.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5973      +/-   ##
==========================================
- Coverage   93.84%   92.81%   -1.04%     
==========================================
  Files         299      250      -49     
  Lines       14417    10352    -4065     
  Branches      385      432      +47     
==========================================
- Hits        13530     9608    -3922     
+ Misses        884      741     -143     
  Partials        3        3              
Impacted Files Coverage Δ
packages/fxa-auth-server/lib/senders/email.js 98.28% <ø> (ø)
...yments-server/src/components/PaymentForm/index.tsx 100.00% <ø> (+1.35%) ⬆️
packages/fxa-payments-server/src/index.tsx 14.03% <ø> (ø)
...ackages/fxa-payments-server/src/lib/AppContext.tsx 80.00% <ø> (ø)
packages/fxa-payments-server/src/lib/apiClient.ts 92.85% <0.00%> (-7.15%) ⬇️
...outes/Subscriptions/Reactivate/ManagementPanel.tsx 100.00% <ø> (ø)
...rver/src/routes/Subscriptions/SubscriptionItem.tsx 90.47% <ø> (-0.44%) ⬇️
packages/fxa-payments-server/src/store/state.ts 100.00% <ø> (ø)
...rofile-server/lib/routes/ecosystem_anon_id/post.js 38.09% <31.57%> (-6.35%) ⬇️
packages/fxa-auth-server/bin/key_server.js 66.17% <50.00%> (-0.99%) ⬇️
... and 76 more

@johngruen johngruen requested a review from lmorchard July 17, 2020 14:46
Copy link
Contributor

@lmorchard lmorchard left a comment

Choose a reason for hiding this comment

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

This looks like it works, but is there a reason why the background can't be a part of the image asset itself? (i.e. we use a shared asset that we can't tailor to subplat pages?)

@lmorchard
Copy link
Contributor

lmorchard commented Jul 17, 2020

Also, good job figuring out how to add a product metadata prop (it's tangly 😅)

<div className="plan-details-logo-wrap">
<div
className="plan-details-logo-wrap"
style={{ ...setWebIconBackground }}
Copy link
Contributor

@lmorchard lmorchard Jul 17, 2020

Choose a reason for hiding this comment

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

I guess if the markup around the icon ever gets more complex, we should consider extracting it out to a shared component that does stuff like the setWebIconBackground logic. But not worth holding this PR up for

@johngruen
Copy link
Contributor Author

@lmorchard WRT setting the background as color, I did this based on the hunch that lead page design will change far more rapidly than will logo design, so having a color element that is easier to swap than uploading a static asset felt right. It also gives us a little more flexibility about placing an svg on an arbitrarily large background if our page layout changes.

@johngruen johngruen merged commit 96161f3 into main Jul 20, 2020
@dannycoates dannycoates deleted the make-product-icon-bg-metadata branch October 29, 2020 06:22
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.

Mozilla VPN icon is missing in the “Thanks!” resubscribe popup
2 participants