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

payment icons updated #413

Closed
wants to merge 23 commits into from
Closed

payment icons updated #413

wants to merge 23 commits into from

Conversation

maunaw
Copy link
Contributor

@maunaw maunaw commented Feb 11, 2021

** Checklist **

  • All icons have a corresponding entry in db/payment_icons.yml
  • I have followed the icon guidelines detailed in the CONTRIBUTING.md file
  • I have optimized the icon with SVGO
  • I am confident that all icons are clear and easy to read/understand
  • I have provided a link to the brand icon’s brand guidelines whenever possible.

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

Resolves #417

Copy link
Collaborator

@larouxn larouxn left a comment

Choose a reason for hiding this comment

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

There are a few issues we need to address.

  • Mastercard does not need to be added, it already exists as master.
  • Why are we updating PayPal, Visa, and Maestro? Seems unnecessary. (also weird spacing)
  • Why are we updating the Afterpay icon? It's currently correct according to https://www.afterpay.com. Do we need a new afterpay icon for a different similarly named service?
  • Why is the Swish icon update blank? (pure white)
  • Why do all icons have an empty space towards the bottom? (see below)
    space

@maunaw
Copy link
Contributor Author

maunaw commented Feb 18, 2021

There are a few issues we need to address.

  • Mastercard does not need to be added, it already exists as master.
  • Why are we updating PayPal, Visa, and Maestro? Seems unnecessary. (also weird spacing)
  • Why are we updating the Afterpay icon? It's currently correct according to https://www.afterpay.com. Do we need a new afterpay icon for a different similarly named service?
  • Why is the Swish icon update blank? (pure white)
  • Why do all icons have an empty space towards the bottom? (see below)
    space

Hi, @larouxn Thank you for the response. Changes have been made as per suggestions. Can you please take a look at latest commits?

@maunaw maunaw requested a review from larouxn February 18, 2021 06:12
@larouxn
Copy link
Collaborator

larouxn commented Feb 22, 2021

👋 Hey @maunaw, can we do the following?

  • Remove the changes to Afterpay, Maestro, PayPal, and Visa
  • Remove the new, unnecessary Mastercard SVG
  • Fix all of the added/updated icons so they have the proper 7% visible gray border (guidelines | template)
  • Run SVGO on all of the added/updated icons (step 5 in here)

@momai-nets
Copy link

momai-nets commented Feb 24, 2021

Hi Nicholas,
Thank you for your feedback.

Remove the changes to Afterpay, Maestro, PayPal, and Visa
Remove the new, unnecessary Mastercard SVG
****- Former versions did not follow the guideline fully because of transparent background. Therefore we are surprised by this**** comment
Fix all of the added/updated icons so they have the proper 7% visible gray border (guidelines | template)

- All icons consists of background#fff and 1px border #000 0.07transparency border-radius 2px ??
Run SVGO on all of the added/updated icons (step 5 in here)
- Please check content of SVG files. All files are fully optimized.

@larouxn
Copy link
Collaborator

larouxn commented Feb 24, 2021

👋 Hello @momai-nets, the updated Afterpay icon is still the wrong icon. It should be the icon for afterpay.com. Additionally, Maestro and other icons are still less tall for some reason. This is not desirable. Can we please remove the Afterpay, Maestro, PayPal, and Visa changes as well as the added Mastercard icon?
Screen Shot 2021-02-24 at 18 48 02
Screen Shot 2021-02-24 at 18 48 12

Additionally it seems the updated icons do not seem to have the proper border style.
Screen Shot 2021-02-24 at 18 51 18
Screen Shot 2021-02-24 at 18 51 31

The Visa icon can be used as a guideline. Newly added/updated icons should adhere to this style. If we look carefully, we can see the updated icons do not have the same border style as the grey border is visible upon the black background.
Screen Shot 2021-02-24 at 18 53 55Screen Shot 2021-02-24 at 18 54 02
Screen Shot 2021-02-24 at 18 57 26Screen Shot 2021-02-24 at 18 58 05

Lastly, I feel SVGO likely has not been run on the icons as SVGO compresses icons to a single line, whereas all of these updated/added icons are multi-line. Though, this is the least of my concerns.


All of that said, I am somewhat okay the changes to the icons other than Afterpay, Maestro, PayPal, Visa, and Mastercard.

@momai-nets
Copy link

Arh Thank you Nicolas,
I see the issue now from your screenshots. The batch of icons you are working with are NOT the latest batch where we have fixed all issues and followed the guidelines fully. We will look into it.

@maunaw
Copy link
Contributor Author

maunaw commented Mar 3, 2021

@larouxn We have updated the icons running SVGO and restoring old ones. Request you to please review the updated icons with latest commit. Kindly let us know if anything. Thank you.

@larouxn
Copy link
Collaborator

larouxn commented Mar 3, 2021

👋 I will try to take a look at this tomorrow. In the meantime, can we remove the mastercard.svg? There's already an icon available at master.svg.

https://github.com/activemerchant/payment_icons/blob/2c77ec6a6d17ab352b411b4d8b5411b2b82e95b2/app/assets/images/payment_icons/master.svg

@maunaw
Copy link
Contributor Author

maunaw commented Mar 3, 2021

👋 I will try to take a look at this tomorrow. In the meantime, can we remove the mastercard.svg? There's already an icon available at master.svg.

https://github.com/activemerchant/payment_icons/blob/2c77ec6a6d17ab352b411b4d8b5411b2b82e95b2/app/assets/images/payment_icons/master.svg

Sure, removed mastercard.svg. Thank you.

Copy link
Collaborator

@larouxn larouxn left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up! Looking good.
Screen Shot 2021-03-05 at 11 10 15

@tauthomas01, can you take a look?

app/assets/images/payment_icons/dankort.svg Outdated Show resolved Hide resolved
app/assets/images/payment_icons/mobilepay.svg Outdated Show resolved Hide resolved
app/assets/images/payment_icons/ratepay.svg Outdated Show resolved Hide resolved
app/assets/images/payment_icons/swish.svg Outdated Show resolved Hide resolved
app/assets/images/payment_icons/vipps.svg Outdated Show resolved Hide resolved
@maunaw
Copy link
Contributor Author

maunaw commented Mar 8, 2021

@tauthomas01 Thank you. Suggested changes are committed to this PR. Request you to please review.

@maunaw maunaw requested a review from tauthomas01 March 8, 2021 11:21
Copy link
Collaborator

@tauthomas01 tauthomas01 left a comment

Choose a reason for hiding this comment

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

Hi,

I left some comments.

Please let me know if there are any questions.

<svg xmlns="http://www.w3.org/2000/svg" width="38" height="24" viewBox="0 0 38 24" role="img" aria-labelledby="pi-dankort"><title id="pi-dankort">Dankort</title><g fill-rule="evenodd" clip-rule="evenodd"><path fill="#FFF" d="M36 24H2c-1.333 0-2-.7-2-2V2C0 .667.7 0 2 0h34c1.333 0 2 .7 2 2v20c0 1.333-.667 2-2 2z"/><path fill="none" stroke="#000" stroke-linecap="round" stroke-linejoin="round" stroke-miterlimit="3" stroke-opacity=".07" d="M36 24c1.333 0 2-.7 2-2V2c0-1.333-.7-2-2-2H2C.7 0 0 .7 0 2v20c0 1.333.7 2 2 2h34z"/></g><path fill-rule="evenodd" clip-rule="evenodd" fill="#E31F28" d="M14.7 10.6c.3.267.384.633.25 1.1-.167.633-.383 1.066-.65 1.3-.333.267-.833.4-1.5.4H8.2l1.15-3.15h4.199c.535 0 .918.117 1.151.35zm5.7 1.85L23.95 17h6.95a8.363 8.363 0 01-2.75 2.2c-1.1.533-2.284.8-3.55.8H11.601c-1.2 0-2.351-.267-3.45-.8A7.727 7.727 0 015.4 17h8.1c2.033 0 3.601-.384 4.7-1.15 1.133-.7 1.866-1.833 2.2-3.4zM24.6 4c1.2 0 2.334.267 3.4.8a7.924 7.924 0 012.7 2h-6.75l-3.4 3.5c-.133-1.233-.733-2.133-1.8-2.7-.967-.533-2.316-.8-4.05-.8H5.55a7.82 7.82 0 012.75-2c1.067-.533 2.167-.8 3.301-.8H24.6zM31 7.25c.5.667.9 1.4 1.2 2.2.267.833.4 1.683.4 2.55 0 .833-.133 1.65-.399 2.45-.2.767-.55 1.45-1.05 2.05l-4.25-5.1L31 7.25z"/></svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The border style needs to follow like other icons

Please follow the contributions guideline

You can follow this example icon as a reference.

alt

app/assets/images/payment_icons/mobilepay.svg Outdated Show resolved Hide resolved
app/assets/images/payment_icons/ratepay.svg Outdated Show resolved Hide resolved
app/assets/images/payment_icons/swish.svg Outdated Show resolved Hide resolved
app/assets/images/payment_icons/vipps.svg Outdated Show resolved Hide resolved
@nrbar
Copy link

nrbar commented Mar 24, 2021

We have updated the icons. Request you to please review the updated icons with latest commit. Kindly let us know if anything. Thank you.

<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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please remove these changes to visa.svg?

<path fill="#3086C8" d="M23.9 8.3l-.2.2c-.5 2.8-2.2 3.8-4.6 3.8H18c-.3 0-.5.2-.6.5l-.6 3.9-.2 1c0 .2.1.4.3.4H19c.3 0 .5-.2.5-.4v-.1l.4-2.4v-.1c0-.2.3-.4.5-.4h.3c2.1 0 3.7-.8 4.1-3.2.2-1 .1-1.8-.4-2.4-.1-.5-.3-.7-.5-.8z"/>
<path fill="#012169" d="M23.3 8.1c-.1-.1-.2-.1-.3-.1-.1 0-.2 0-.3-.1-.3-.1-.7-.1-1.1-.1h-3c-.1 0-.2 0-.2.1-.2.1-.3.2-.3.4l-.7 4.4v.1c0-.3.3-.5.6-.5h1.3c2.5 0 4.1-1 4.6-3.8v-.2c-.1-.1-.3-.2-.5-.2h-.1z"/>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please remove these changes to paypal.svg?

<circle fill="#EB001B" cx="15" cy="12" r="7"/>
<circle fill="#F79E1B" cx="23" cy="12" r="7"/>
<path fill="#FF5F00" d="M22,12c0-2.4-1.2-4.5-3-5.7c-1.8,1.3-3,3.4-3,5.7s1.2,4.5,3,5.7C20.8,16.5,22,14.4,22,12z"/>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please remove this duplicate mastercard.svg? (already one under master.svg)

<circle fill="#00A2E5" cx="23" cy="12" r="7"/>
<path fill="#7375CF" d="M22 12c0-2.4-1.2-4.5-3-5.7-1.8 1.3-3 3.4-3 5.7s1.2 4.5 3 5.7c1.8-1.2 3-3.3 3-5.7z"/>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please remove these changes to maestro.svg?

c0.366-0.183,0.731,0.092,0.731,0.458v0.366c0,1.921,2.105,3.11,3.753,2.195l2.47-1.464l2.47-1.464
C28.293,10.628,28.293,8.249,26.645,7.242z"/>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please remove these changes to afterpay.svg?
Screen Shot 2021-03-26 at 16 35 56
Screen Shot 2021-03-26 at 16 36 00
Source: https://www.afterpay.com/index

L11.614,7.892c-0.13-0.337-0.125-0.68,0.016-1.028c0.141-0.337,0.386-0.582,0.734-0.734l4.879-2.023
c0.359-0.142,0.702-0.142,1.028,0C18.631,4.258,18.881,4.503,19.023,4.84z"/>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be able to drop this change in favour of #424.

@maunaw maunaw marked this pull request as draft May 7, 2021 05:52
@maunaw
Copy link
Contributor Author

maunaw commented May 7, 2021

Closing this pull request as created a new one with exact changes required.

@maunaw maunaw closed this May 7, 2021
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.

payment icons updated
5 participants