Skip to content

Add a new icon for zaver #1272

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

Merged
merged 6 commits into from
Apr 3, 2025
Merged

Conversation

divyach0209
Copy link
Contributor

@divyach0209 divyach0209 commented Jan 2, 2025

Why are you adding this icons?

I'm adding/updating this icon(s) because our payment gateway offer a new payment method called Zaver.

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:

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.

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

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?

@divyach0209
Copy link
Contributor Author

Hello @Lydia-shan-git
could you please help to review this as soon as possible as due to a customer requirement we need to use it on our shopify app.

@divyach0209
Copy link
Contributor Author

Hi @dannye0231
Hope you are doing well. Could you please help me out to get this PR reviewed as soon as possible as whole feature is build and just waiting for this PR to be rollout to make the release to have the included shopify icon or method visible.

@dannye0231
Copy link
Contributor

Hi @divyach0209 , we are aiming for Feb 5th to get these PRs merged.

Can you please re-base as well?

@divyach0209
Copy link
Contributor Author

sure thanks for the update @dannye0231 :)

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.

Please make sure your border is 1 px thick and follows the radius of 2 px as shown in the guidelines (https://github.com/activemerchant/payment_icons/blob/master/CONTRIBUTING.md#appearance)

@divyach0209
Copy link
Contributor Author

Hi @Lydia-shan-git
Thanks for taking this into consideration. I have updated the icon with specific border and radius as per the guidelines and have rebased with latest changes too @dannye0231.

So could you please help to re-review the same ? and please let me know if anything required from my side.

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.

Thank you for the changes, just make sure that your logo does not overlap with the border. It should stay within the border

@divyach0209
Copy link
Contributor Author

Hi @Lydia-shan-git
Thank you for responding back. I have fixed the border overlapping too here could you please re-review the same and help to process this further.

@dannye0231
Copy link
Contributor

@divyach0209 are you able to check your SVG for the following issue?

Failure:
PaymentIconTest#test_Every_payment_SVG_meets_accessibility_requirements [test/unit/payment_icon_test.rb:74]:
{:message=>"The 'zaver' SVG file should have a <title> tag as first child of the root tag"}.
Expected: "title"
Actual: "rect"

@divyach0209
Copy link
Contributor Author

Hi @dannye0231
Have fixed the title placement and rebase with upstream too for resolving conflicts. Could you please help re-review and proceed now. Will be really helpful if we could be able to merge this week only.

@divyach0209
Copy link
Contributor Author

Hi @dannye0231 @Lydia-shan-git
Hope you are doing well!
Could you please help me to re-review and proceed on this PR as it's halting production deployment of a payment method included with icon which is done.

@dannye0231
Copy link
Contributor

Hi @divyach0209 , it looks good from my side now. We'll need our UX team to approve before it can get merged. Our next merge window (once approved) will be in early March.

@divyach0209
Copy link
Contributor Author

divyach0209 commented Feb 17, 2025

Hi @dannye0231
Could you please help me to atleast get an approval on this so that if anything needs to be update then I can be on top of that and it won't get delay anymore.

@divyach0209
Copy link
Contributor Author

Hi @Lydia-shan-git @dannye0231
Can we please get an update on this ? We are waiting to launch the payment method and stuck with the icon. Could you please help out here ?

@dannye0231
Copy link
Contributor

Hi @divyach0209 , we're aiming to have everything ready for Wednesdays merge run.

@divyach0209
Copy link
Contributor Author

Hi @dannye0231
Thank you so much for getting back to me. As right now it shows no conflicts with base branch. So if you think anything needs to be fix on prior please do let me know so I can make the changes ready before wednesday :)

@divyach0209
Copy link
Contributor Author

Hi @dannye0231 @Lydia-shan-git
Is there any updates on the same ? Could you please help me out on status when we can expect this merge or if there is anything needs to be fix could you please help me to share before your merge window would be really great.

@dannye0231
Copy link
Contributor

@divyach0209 we are aiming for this Wednesday to start the merge. I am just working on getting the last approval needed

@divyach0209
Copy link
Contributor Author

Hey @dannye0231
Could you please help out us to get an update here ?

@renjith-mondido
Copy link

Hi ,

We will be really grateful, if you can inform us the plan, so we can inform our integration parters about the time plan,

@dannye0231
Copy link
Contributor

Hi @renjith-mondido , we ran into some delays on our end and will most likely be processing your PR in the first week of April.

@dannye0231
Copy link
Contributor

@divyach0209 can you please check the following:

Visible Border: ❌ (The outer rectangle has a stroke color of "0000" which is invalid; it should be a hex color like #000 or transparent if no border is desired)
Border Color: ❌ (The stroke color should be specified correctly)

Correct the stroke color of the outer rectangle to a valid hex color (e.g., #000 for black)
Ensure the border width is set to 1px

@divyach0209
Copy link
Contributor Author

Hi @dannye0231
Have made the changes as required could you please have a look on.

cc : @renjith-mondido

@dannye0231
Copy link
Contributor

@divyach0209 can you please check the following based on our requirements?

Opacity Requirement:

Opacity Definition: Check if the SVG has an opacity attribute set for any elements. If not, ensure that the design does not require any transparency.
Status: ❌ (No opacity set; review contribution guidelines for specific requirements.)

@divyach0209
Copy link
Contributor Author

Hi @dannye0231
I have checked that the SVG does not use any transparency effects. All elements have solid colors, and no opacity settings are required for the intended design. Let me know if a specific opacity level is mandated by the guidelines but for now never used any opacity element and after cross checking guidelines document never found that opacity is required too.
Please let me know if there is any need of the same.

Also it would be really great if we got this merged on this time as we were waiting for the same since January.

@dannye0231
Copy link
Contributor

Please refer to our contribution guide regarding the opacity requirement:

Whenever possible, the border should be black (hex color #000) with a 7% opacity (0.07).

@divyach0209
Copy link
Contributor Author

Hi @dannye0231
Thanks for reply back. Added opacity and colour accordingly could you please re-review the same and let me know if anything else required.

Copy link
Contributor

@dannye0231 dannye0231 left a comment

Choose a reason for hiding this comment

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

SVG Code Review

Root Tag Attributes:
xmlns: ✅ Correctly included (xmlns="http://www.w3.org/2000/svg").
role: ✅ Correctly set (role="img").
viewBox: ✅ Present and correct (viewBox="0 0 38 24").
width and height: ✅ Present and correct (width="38" and height="24").
aria-labelledby: ✅ Correctly references the <title> element (aria-labelledby="pi-zaver").

Nested <title> Tag:
id: ✅ Correctly set to id="pi-zaver".

Inner text: ✅ Appropriately describes the icon (<title>Zaver</title>).

Standard SVG Structure:
✅ The SVG follows a proper structure with nested elements.

Vector Graphics Requirement:
✅ The SVG consists of vector graphics elements only, using and elements.

Embedded Fonts and Bitmaps:
✅ No embedded fonts or bitmap images are present.

ID Attributes:
✅ No conflicting ID attributes; all IDs follow the pi- naming convention.

Inline Styles:
✅ No <style> tags are present; styles are applied inline.

Base64 Encoded Images:
✅ No base64-encoded images are present, which is acceptable.

Class Attributes:
✅ No class attributes are present in the SVG.

Logos and Backgrounds:
✅ The SVG contains a background rectangle with a white fill and a black stroke with low opacity.

Clarity and Readability:
✅ The icon appears clear and recognizable.

Accessibility:
✅ The <title> element enhances accessibility for screen readers.

Structure:
✅ The SVG structure is organized and properly nested.

Metadata:
✅ There are no additional elements, but this is not mandatory.

@dannye0231 dannye0231 merged commit 0a80d50 into activemerchant:master Apr 3, 2025
8 checks passed
@divyach0209
Copy link
Contributor Author

Hi @dannye0231
Thank you so much for merging the PR :)
I tried to allow the same method on our staging app but getting issue from shopify that :
╭─ error ──────────────────────────────────────────────────────────────────────╮
│ │
│ Version couldn't be created. │
│ │
│ payment-gateway │
│ │
│ Validation errors │
│ • supported_payment_methods: Following payment methods not permitted: │
│ ["zaver"] │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯

So is it like we have to wait for the deployment now ?

It would be really great if you could help us with the same.

@dannye0231
Copy link
Contributor

dannye0231 commented Apr 3, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants