-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add PaymentIcon to Checkout #1340
Conversation
2dc03a0
to
a7ea6cb
Compare
/** | ||
* The name of the payment method. | ||
* | ||
* Check the list of available payment methods [here](https://github.com/activemerchant/payment_icons/tree/master/app/assets/images/payment_icons). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed the link. We should point them to the PaymentMethod
type no?
I don't believe the names of the icons in the the active_merchant repo matches the ones we accept here, so I'd remove this link to avoid any confusion. Developers should only refers to the list of PaymentMethod listed above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the link. Do you know why the docs only show "string" even though the type is PaymentMethod | string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs need to be generated manually.
@oliverigor Can you help @lsit with the docs? Do we have the whole flow documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified that it appears to have a bug in the script/platform. it appears that it's not accepting those two types together.
Lianne followed the correct docs, but the script is not picking up the correct types.
But what is strange to me is that, why are we allowing any string and some defined PaymentMethods? should't we just allow just PaymentMethod
? testing with that I could see that the script works just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I asked about it in #core-build-learn. They were able to reproduce the issue and logged an issue on their end for it https://github.com/Shopify/generate-docs/issues/217
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be shipped as is and it'll be fixed eventually or should I wait for their fix? For context, I'm trying to get this component exposed for the 2023-10 release and the cutoff date is next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead. The docs can be fixed after, or manually modified if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that because PaymentMethod
encompasses a list of strings, so as far as typescript is concerned, the type is now just "string" since it can be a list of the PaymentMethod's string or a string which ultimately results in a string. The #core-build-learn team's suggestion was to add the definition of PaymentMethod to the page, which I've done now.
Background
Resolves https://github.com/Shopify/checkout-web/issues/25226
Solution
Copied the types and docs from private packages
🎩
https://shopify-dev.payment-icon.lianne-sit.us.spin.dev/docs/api/checkout-ui-extensions/unstable/components/media/paymenticon
Checklist