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

feat: Add new icons for efecty, nequi and yape, also fix Visa icon th… #1221

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

Conversation

Candres1019
Copy link

Why are you adding this icons?

Added new icons for Nequi, Efecty, and Yape—Colombian and Peruvian payment methods, respectively. These icons will be used by PayU. Also, corrected inconsistencies and defects in the Visa logo, ensuring uniformity in letter colors and clarity.

Help us identify yourself

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

Colombia:

Nequi - https://www.nequi.com.co/
Efecty - https://www.efecty.com.co/web/

Peru

Yape - https://www.yape.com.pe/

Global

Visa - https://www.merchantsignage.visa.com/brand_guidelines

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.
  • I have attached a screenshot comparison with the example icon provided in guidelines
  • I recognize that if my icon is not approved by the Shopify Partners team it may not receive review nor merger.

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

image

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

…at had a wrong visual in the letters, also fix the visa brand color for the proper one
@adeniyiao
Copy link
Contributor

Hi @Candres1019,

Kindly address the test failure as seen below

PaymentIconTest#test_Every_payment_SVG_meets_accessibility_requirements [test/unit/payment_icon_test.rb:89]:
{:message=>"The 'Capa_1' ID should be pi-efecty-Capa_1 (missing 'pi-' prefix)"}.
Expected /pi-(.*)/ to match "Capa_1".

@Candres1019
Copy link
Author

Hi @adeniyiao ,

Thanks for the comments, i just made the changes to add the pi preffix in the ids

@dani-ooo
Copy link

dani-ooo commented Sep 2, 2024

hello. i'm not very experienced with pull requests, so i'm wondering if this will be an issue:
i have a previous open pull request updating the Visa icon as well: #1210.

i also have (i believe) slightly better icons for Nequi and Efecty ready, but i didn't submit because i wasn't sure whether they'd be approved based on the Shopify Partners disclaimer. how could i propose those, without affecting this pull request?

@dannye0231
Copy link
Contributor

Hi @Candres1019 , are you able to check the SVGs and correct the naming errors when are coming up in the tests? I already fixed a few issues within the YML file and and Visa icon but there are still additional errors coming up in the remaining SVGs.

@Candres1019
Copy link
Author

Hi @dannye0231 , thank u , i just did the changes that are mention in the test

@dannye0231
Copy link
Contributor

Hi @Candres1019 , are you able to check one more error that is showing up?

"The 'Capa_1' ID should be pi-yape-Capa_1 (missing 'pi-' prefix)"}.

@Candres1019
Copy link
Author

Hi @dannye0231 , sure, done

@LauraZaratePayU
Copy link

LauraZaratePayU commented Oct 17, 2024

Hi @Lydia-shan-git , Have you managed to carry out this review to move forward with the inclusion of new icons?

@Candres1019
Copy link
Author

Hi @Lydia-shan-git , can we do the merge of this branch ?

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.

Looks good, just pass each icon into SVGO to reduce it's size

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dannye0231 We're not changing major card icons right?

Copy link
Contributor

@dannye0231 dannye0231 Nov 22, 2024

Choose a reason for hiding this comment

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

@Lydia-shan-git you are correct, these PRs would only apply to the individual payment integration icons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Candres1019 , are you able to omit the Visa icon change from this PR and only keep your payment integration icon in scope?

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.

6 participants