payment_icons icon indicating copy to clipboard operation
payment_icons copied to clipboard

Add Best Buy Card

Open aubreytayloracn opened this issue 10 months ago • 6 comments

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

  • [X] I'm working/collaborating with the brand directly and they have provided the icons.
  • [ ] I'm associated with the brand and I've read all the brand icon’s guidelines.
  • [ ] I'm an individual and I've read all the brand icon’s guidelines.

Link to the brand guidelines: <Link>

Checklist to add new icons

  • [X] All icons have a corresponding entry in db/payment_icons.yml
  • [X] I have followed the icon guidelines detailed in the CONTRIBUTING.md file
  • [X] I have optimized the icon with SVGO
  • [X] I am confident that all icons are clear and easy to read/understand
  • [X] I have provided a link to the brand icon’s brand guidelines whenever possible.
  • [X] I have attached a screenshot comparison with the example icon provided in guidelines
  • [X] I recognize that if my icon is not approved by the Shopify Partners team it may not receive review nor merger.

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)

  • Altmash Shaikh, Sunjay Chopra

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

  • 5/1/2024

aubreytayloracn avatar Apr 03 '24 00:04 aubreytayloracn

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?

hellicarusprime avatar Apr 08 '24 10:04 hellicarusprime

@hellicarusprime:

  • Re: 2 cards overlapping – This was the card art I was provided. I'll circle back with the team. Is there payment icon design guidelines I can reference or do you have specific feedback?
  • Re: Gradient fill – I will circle back with the team on this one, it may be difficult to get approval to modify the lockup design too much.

aubreytayloracn avatar Apr 08 '24 16:04 aubreytayloracn

@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)?

aubreytayloracn avatar Apr 08 '24 18:04 aubreytayloracn

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.

hellicarusprime avatar Apr 09 '24 09:04 hellicarusprime

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

aubreytayloracn avatar Apr 09 '24 17:04 aubreytayloracn

@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.

aubreytayloracn avatar Apr 10 '24 18:04 aubreytayloracn

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

adeniyiao avatar Apr 11 '24 12:04 adeniyiao

@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 avatar Apr 11 '24 17:04 aubreytayloracn

@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

aubreytayloracn avatar Apr 12 '24 20:04 aubreytayloracn

@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.

Lovedanihonjin avatar Apr 15 '24 20:04 Lovedanihonjin

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 avatar Apr 16 '24 01:04 aubreytayloracn

@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!

aubreytayloracn avatar Apr 17 '24 18:04 aubreytayloracn

@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"

adeniyiao avatar Apr 17 '24 18:04 adeniyiao

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

aubreytayloracn avatar Apr 17 '24 19:04 aubreytayloracn

@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

adeniyiao avatar Apr 17 '24 20:04 adeniyiao

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

aubreytayloracn avatar Apr 18 '24 23:04 aubreytayloracn

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

aubreytayloracn avatar Apr 19 '24 18:04 aubreytayloracn

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 avatar Apr 22 '24 18:04 aubreytayloracn

@Lovedanihonjin – 🙌 Awesome thank you!

aubreytayloracn avatar Apr 22 '24 19:04 aubreytayloracn