Closed rakholiyasaurav535 closed 7 months ago
I've added the Ruby 2.7 rails edge to be excluded as mentioned in #1121
Hi @rakholiyasaurav535 we addressed the ruby version issue, but we need you to rebase your branch rakholiyasaurav535:taly_new_icon
with activemerchant/master
as well. Also the test fails due to issue with SVG as described in screenshot below
Hey @adeniyiao I've made the changes and svg file and rebased my branch as well, please review it ASAP
Hey @adeniyiao I've made the changes and svg file and rebased my branch as well, please review it ASAP
HI @rakholiyasaurav535, there's another test failing with the svg as seen below. Kindly look into it. Thanks
PaymentIconTest#test_Every_payment_SVG_meets_accessibility_requirements [/home/runner/work/payment_icons/payment_icons/test/unit/payment_icon_test.rb:89]:
{:message=>"The 'paint0_linear_4947_4625' ID should be pi-taly-paint0_linear_4947_4625 (missing 'pi-' prefix)"}.
Expected /pi-(.*)/ to match "paint0_linear_4947_4625".
Hey @adeniyiao I've added the pi prefix to the ID
Hi @rakholiyasaurav535 ,
Another error
Failure: PaymentIconTest#test_Every_payment_SVG_meets_accessibility_requirements [/home/runner/work/payment_icons/payment_icons/test/unit/payment_icon_test.rb:93]: {:message=>"The 'taly' SVG file should have a 'role' attribute on the root <svg> tag"}
Hey @adeniyiao
I've added the role attribute ran the github action locally to verify if there's any other change to be made and seems like there's no more, hope this get's merge ASAP now.
Hi @rakholiyasaurav535 I noticed you're using a gradient fill on the background of the icon. Would it be possible to change this to a solid background color? We request this in our guidelines whenever possible. Have you also considered adding the border that we suggest there?
Hey @hellicarusprime We've updated the svg as per the requirements
Hey @hellicarusprime We've updated the svg as per the requirements
Thanks for the update @rakholiyasaurav535, but it looks like you've got a 4px border-radius now instead of the required 2px.
Hey @hellicarusprime, Thanks for pointing out the border-radius issue, I've adjusted it to 2px as needed.
Hey @hellicarusprime, Thanks for pointing out the border-radius issue, I've adjusted it to 2px as needed.
Hey @rakholiyasaurav535 It doesn't look like it's changed.
Hey @hellicarusprime check now
@rakholiyasaurav535 You have a different issue now. Although the icon is 38 x 24, it's displaying too small due to the fact that it doesn't have a border. I've attached an example of what we need to help explain.
hey @hellicarusprime check now
@rakholiyasaurav535 This is much better! One last thing, it looks like you can reduce the file size by ~55% if you run it through SVGO. Once this is done, I'll be happy to approve. Thanks!
@hellicarusprime I've reduced the size
@hellicarusprime Is there anything pending from our end now?
@hellicarusprime Is there anything pending from our end now?
@miteshpanchal1992 The icon is good, but it looks like there are some branch conflicts. Cc: @adeniyiao
Hi @miteshpanchal1992 Could you please rebase this branch with activemerchant/master to resolve the conflicts. Thanks.
The next release is planned for Feb 7.
hey @adeniyiao I've rebased the branch, do let me know if there is anything else pending from our end
Hi @rakholiyasaurav535 , it looks like the rebase was not done correctly as there are other files in the PR this PR that shouldn't be included as they are already merged. Could you please review and correct. Thanks
Hey @adeniyiao check now
Still doesn't look right. May be close the PR and open another one as you will see that you have more files (https://github.com/activemerchant/payment_icons/pull/1120/files) than expected in this PR
Hey @adeniyiao
I've created a new Pull Request please get this merged ASAP
Why are you adding this icons?
I'm adding/updating this icon(s) because we are developing an offsite payment app in shopify.
Help us identify yourself
Link to the brand guidelines:
Checklist to add new icons
db/payment_icons.yml
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
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?