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

[Managed Markets] Adding new payment icon for KCP Credit Card and updating Vipps payment icon #1244

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mseal-shopify
Copy link
Contributor

@mseal-shopify mseal-shopify commented Oct 21, 2024

This closes: https://github.com/Shopify/markets-pro-payments/issues/1899

Why are you adding this icons?

I'm adding a new payment icon for KCP. I used the SVGO UI to optimise the icon. https://checkoutshopper-live.adyen.com/checkoutshopper/images/logos/kcp_creditcard.svg

I'm also updating Vipps icon to latest icon. https://checkoutshopperlive.adyen.com/checkoutshopper/images/logos/vipps.svg

Checklist to add new icons

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

@mseal-shopify mseal-shopify force-pushed the mm/kcp_credit_card_icon branch 2 times, most recently from d221782 to 523e93a Compare October 21, 2024 05:49
@mseal-shopify mseal-shopify changed the title [Managed Markets] Adding payment icon for KCP Credit Card [Managed Markets] Adding new payment icon for KCP Credit Card and updating Vipps payment icon Oct 21, 2024
Copy link
Collaborator

@Lydia-shan-git Lydia-shan-git left a comment

Choose a reason for hiding this comment

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

Make sure the entire logo fits within the borders. Currently, you have a green square that overlaps on top of the border by a smidge. Other than everything else looks good

@mseal-shopify mseal-shopify force-pushed the mm/kcp_credit_card_icon branch from c84ff87 to 6b936cd Compare October 22, 2024 03:40
Copy link
Collaborator

@Lydia-shan-git Lydia-shan-git left a comment

Choose a reason for hiding this comment

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

Looking good! Just run it through SVGO so it's the smallest possible file size

@adeniyiao
Copy link
Contributor

Hi @mseal-shopify , just a quick observation, for kcp_creditcard, the icon name and filename should be lowercase alpha characters only as per the guide.

Thanks

@mseal-shopify
Copy link
Contributor Author

Hi @mseal-shopify , just a quick observation, for kcp_creditcard, the icon name and filename should be lowercase alpha characters only as per the guide.

Thanks

Thanks pointing that out, but I see there are other icons as well with underscore in their names. Is that something that's not allowed?

@adeniyiao
Copy link
Contributor

Hi @mseal-shopify , sorry for the late response. Previously underscore was accepted for the old icons, but new icons should not have underscore as per latest guide.

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.

4 participants