activemerchant / payment_icons

An easy to use library that allows you to manage and access payment icons
MIT License
142 stars 409 forks source link

Add Best Buy Card #1160

Closed aubreytayloracn closed 2 months ago

aubreytayloracn commented 3 months ago

Why are you adding this icons?

I'm adding/updating this icon(s) because they are required for the Best Buy Card Shopify integration

Help us identify yourself

Link to the brand guidelines:

Checklist to add new icons

If this pull request is not adding new icons, you can remove this checklist.

Attach a screenshot of the icon along side the example Visa icon

Tips how to create a screenshot

We have found free online SVG editor https://www.freecodeformat.com/svg-editor.php very useful to create one. Here is a sample code for you to verify that you icon appears properly along side the placeholder.

<!-- Change background color if needed to showcase your icon better -->
<style> body { background: black; } </style>

<!-- DO NOT DELETE EXAMPLE -->
<svg viewBox="0 0 38 24" xmlns="http://www.w3.org/2000/svg" role="img" width="38" height="24" aria-labelledby="pi-visa"><title id="pi-visa">Visa</title><path opacity=".07" d="M35 0H3C1.3 0 0 1.3 0 3v18c0 1.7 1.4 3 3 3h32c1.7 0 3-1.3 3-3V3c0-1.7-1.4-3-3-3z"/><path fill="#fff" d="M35 1c1.1 0 2 .9 2 2v18c0 1.1-.9 2-2 2H3c-1.1 0-2-.9-2-2V3c0-1.1.9-2 2-2h32"/><path d="M28.3 10.1H28c-.4 1-.7 1.5-1 3h1.9c-.3-1.5-.3-2.2-.6-3zm2.9 5.9h-1.7c-.1 0-.1 0-.2-.1l-.2-.9-.1-.2h-2.4c-.1 0-.2 0-.2.2l-.3.9c0 .1-.1.1-.1.1h-2.1l.2-.5L27 8.7c0-.5.3-.7.8-.7h1.5c.1 0 .2 0 .2.2l1.4 6.5c.1.4.2.7.2 1.1.1.1.1.1.1.2zm-13.4-.3l.4-1.8c.1 0 .2.1.2.1.7.3 1.4.5 2.1.4.2 0 .5-.1.7-.2.5-.2.5-.7.1-1.1-.2-.2-.5-.3-.8-.5-.4-.2-.8-.4-1.1-.7-1.2-1-.8-2.4-.1-3.1.6-.4.9-.8 1.7-.8 1.2 0 2.5 0 3.1.2h.1c-.1.6-.2 1.1-.4 1.7-.5-.2-1-.4-1.5-.4-.3 0-.6 0-.9.1-.2 0-.3.1-.4.2-.2.2-.2.5 0 .7l.5.4c.4.2.8.4 1.1.6.5.3 1 .8 1.1 1.4.2.9-.1 1.7-.9 2.3-.5.4-.7.6-1.4.6-1.4 0-2.5.1-3.4-.2-.1.2-.1.2-.2.1zm-3.5.3c.1-.7.1-.7.2-1 .5-2.2 1-4.5 1.4-6.7.1-.2.1-.3.3-.3H18c-.2 1.2-.4 2.1-.7 3.2-.3 1.5-.6 3-1 4.5 0 .2-.1.2-.3.2M5 8.2c0-.1.2-.2.3-.2h3.4c.5 0 .9.3 1 .8l.9 4.4c0 .1 0 .1.1.2 0-.1.1-.1.1-.1l2.1-5.1c-.1-.1 0-.2.1-.2h2.1c0 .1 0 .1-.1.2l-3.1 7.3c-.1.2-.1.3-.2.4-.1.1-.3 0-.5 0H9.7c-.1 0-.2 0-.2-.2L7.9 9.5c-.2-.2-.5-.5-.9-.6-.6-.3-1.7-.5-1.9-.5L5 8.2z" fill="#142688"/></svg>

<!-- TODO: insert your icon here -->
<YOUR SVG CODE>

<br>
<!-- TODO: insert your icon here -->
<YOUR SVG CODE>
</br

image

If the icons are intended for use by Shopify, please provide the following info:

Who are you working with at Shopify? (avoid adding personal details, provide github handle(preferred) or first name and last name)

What's the expected date of this change to deploy on Shopify?

hellicarusprime commented 3 months ago

Hi @aubreytayloracn, what's the rationale for having 2 cards layered like this? It makes it very difficult to understand what I'm looking at. Could you also avoid using a gradient fill, as detailed in our contributing guidelines?

aubreytayloracn commented 3 months ago

@hellicarusprime:

aubreytayloracn commented 3 months ago

@hellicarusprime  – Just getting a temperature here. We know the card art isn't super readable, is this a hard stop on getting this merged or can we proceed with the icon as is (without the gradient fill)?

hellicarusprime commented 3 months ago

Hi @aubreytayloracn, here are our guidelines for icon appearance. The one I'll call out is that icons are clear and easy to read/understand.

Looping in @adeniyiao to get is opinion on whether this is blocking or not.

aubreytayloracn commented 3 months ago

@hellicarusprime – Thanks for this, I missed it when I last read the guidelines. I've relayed this information.

aubreytayloracn commented 3 months ago

@adeniyiao – Any feedback on the icon design here? Is it a blocker? Getting a new, approved design will be difficult under the current timeline constraints we're facing.

adeniyiao commented 3 months ago

Hi @aubreytayloracn , similar call out is that the design doesn't appear clear and readable. Could you please look into that.

Also the automated test failed with error below

PaymentIconTest#test_Every_payment_SVG_meets_accessibility_requirements [test/unit/payment_icon_test.rb:68]:
{:message=>"The 'bestbuycard' SVG file should have a single <title> tag"}.
Expected: 1
  Actual: 0

Thanks

aubreytayloracn commented 3 months ago

@adeniyiao – Thanks for the error report, I'll fix that.

My question to @hellicarusprime that he tagged you on was actually, is: Can we merge the design as-is or is it a blocker to get this merged.

aubreytayloracn commented 3 months ago

@hellicarusprime @adeniyiao

Design has come back with this as an alternative. Would either or both of these be acceptable (in the correct format of course, with the border etc.)? We prefer the one on the right, if possible.

I'll get the SVG next week, but wanted to float this by to get a 👍 or 👎

image

Lovedanihonjin commented 3 months ago

@hellicarusprime @adeniyiao

Design has come back with this as an alternative. Would either or both of these be acceptable (in the correct format of course, with the border etc.)? We prefer the one on the right, if possible.

I'll get the SVG next week, but wanted to float this by to get a 👍 or 👎

image

Visually I like the right. However in context the 3 dots may not be very legible as the presentation of the payment method is quite small. I would recommend the left.

aubreytayloracn commented 3 months ago

Thanks @Lovedanihonjin 😃 cc: @hellicarusprime @adeniyiao.

I've updated the icon with the 3-dot version (I think it looks ok at the required dimensions) in the PR and updated the description above with a new screenshot. Please let me know if there's any feedback.

aubreytayloracn commented 3 months ago

@Lovedanihonjin , @hellicarusprime , @adeniyiao

Any chance I can get a (hopefully) final review / approval on this? I'd like to get this approved for the 5/1 merge as soon as possible. Thanks!

adeniyiao commented 3 months ago

@Lovedanihonjin , @hellicarusprime , @adeniyiao

Any chance I can get a (hopefully) final review / approval on this? I'd like to get this approved for the 5/1 merge as soon as possible. Thanks!

Hi @aubreytayloracn ,

There's an test failure due to the error below. Could you please address. Thanks

{:message=>"The 'bestbuycard' SVG file does not have the appropriate <title> value"}.
Expected: "BestBuy Card"
  Actual: "Best Buy Card"
aubreytayloracn commented 3 months ago

@adeniyiao – I think I've addressed the issue by fixing the label in payment_icons.yml. Let me know! Thanks!

adeniyiao commented 3 months ago

@adeniyiao – I think I've addressed the issue by fixing the label in payment_icons.yml. Let me know! Thanks!

All passed now, Thanks, @Lovedanihonjin to review

aubreytayloracn commented 3 months ago

@Lovedanihonjin – Updated! Let me know if there's any additional changes, thanks!

aubreytayloracn commented 3 months ago

@Lovedanihonjin @adeniyiao @hellicarusprime – Happy Friday! :) Would it be possible to get a 👍 ? Thanks!

aubreytayloracn commented 3 months ago

Hey all, just wondering if I can get a 👍 or additional changes. Thanks! Want to make sure we can get this in for 5/1 merge.

aubreytayloracn commented 3 months ago

@Lovedanihonjin – 🙌 Awesome thank you!