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 useScriptPaypal #290

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: add useScriptPaypal #290

wants to merge 9 commits into from

Conversation

OrbisK
Copy link

@OrbisK OrbisK commented Oct 8, 2024

Note

This PR is still WIP, opening for (hopefully) early feedback

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR is to add a new script for paypal.

I have tried my hand at writing a script for paypal. There are a few points where I am still unsure. On the one hand with my current implementation of useScriptPaypal(), on the other hand with the possible splitting into different componetns. E.g. <ScriptPaypalButtons>. I have roughly listed a few things below that I would appreciate feedback on.

  1. is it better to load the paypal script from the npm registry or via the url? (versioning?)
  2. useScriptPayPal or useScriptPaypal
  3. query serialization (keys with kebab-case, arrays)
  4. maybe it makes sense to split the single paypal component into different possible components like <ScriptPaypalButtons /> and/or . To get the source code for buttons, the query components=buttonsis passed, for messagescomponents=messages. However, it would probably make more sense to pass components=buttons,messages` and only make the request once. How could this look structurally?

I would then add documentation as progress continues

Note

src/runtime/components/ScriptPayPal.vue is currently just for testing paypal buttons.

Copy link

vercel bot commented Oct 8, 2024

@OrbisK is attempting to deploy a commit to the Nuxt Team on Vercel.

A member of the Team first needs to authorize it.

@harlan-zw
Copy link
Collaborator

harlan-zw commented Oct 16, 2024

Hi, thank you for your hard work on this and sorry for the delay. Overall is looking great.

  1. is it better to load the paypal script from the npm registry or via the url? (versioning?)

We want to try and avoid adding node_modules for end users for scripts they don't need as the module size will grow rapidly, so we prefer loading via URL. This gets a bit tricky for the types though.

useScriptPayPal or useScriptPaypal

useScriptPayPal

  1. query serialization (keys with kebab-case, arrays)

Hmm there may be ways to clean this up but if this is working then I'm fine with what you have.

  1. maybe it makes sense to split the single paypal component into different possible component

If we have or may have multiple components for one provider I think better to name them clearer. I'm not exactly sure if I follow what your saying otherwise as I haven't implemented the PayPal button myself, happy to defer to your judgement there.

@OrbisK
Copy link
Author

OrbisK commented Oct 29, 2024

If we have or may have multiple components for one provider I think better to name them clearer. I'm not exactly sure if I follow what your saying otherwise as I haven't implemented the PayPal button myself, happy to defer to your judgement there.

Unfortunately I have to ask you more specifically, not about the paypal script, but about the way paypal implements it.

The same script may have several functionalities, depending on which one is requested. But this also means that if I want to use different paypal components in two places, then I would request 90% of the code twice. However, it would also be possible to simply pass both components directly to the request.

Another possibility would be to always request all β€œcomponents” from paypal, but then you would have loaded 20% more script than necessary from paypal if you only use one.

It would be best if you had a way of knowing which components there should be before the specific request and thus customize the query parameters granularly.

Hope that makes sense so far.

@OrbisK
Copy link
Author

OrbisK commented Nov 4, 2024

When I thought more about it, I decided to simply request all components first. Maybe you could think about opting out via config

@OrbisK
Copy link
Author

OrbisK commented Nov 18, 2024

Hi, thank you for your hard work on this and sorry for the delay. Overall is looking great.

  1. is it better to load the paypal script from the npm registry or via the url? (versioning?)

We want to try and avoid adding node_modules for end users for scripts they don't need as the module size will grow rapidly, so we prefer loading via URL. This gets a bit tricky for the types though.

useScriptPayPal or useScriptPaypal

useScriptPayPal

  1. query serialization (keys with kebab-case, arrays)

Hmm there may be ways to clean this up but if this is working then I'm fine with what you have.

  1. maybe it makes sense to split the single paypal component into different possible component

If we have or may have multiple components for one provider I think better to name them clearer. I'm not exactly sure if I follow what your saying otherwise as I haven't implemented the PayPal button myself, happy to defer to your judgement there.

okay, the paypal sdk is completely cursed. but i think i have a theoretical progress that shows in which direction it goes. I had not yet done the renaming to usePayPal. I would tackle that and the docs soon.

Unfortunately I don't have a concrete project at the moment where I can test live. The paypal docs are also very wild πŸ˜ƒ some of the types don't match the docs either.

I have worked around the problem with the components as described above.

if you say that everything still fits so far, I would then catch up on the refactoring and docs and try to use everything live in a project πŸ˜„

EDIT:
i have also started to add the card fields. but they are a bit more complex and even less well documented than the rest. I would put them at the back of the list for now.

image

What about tests here? I haven't seen anything about the components yet. I would then add them too

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.

2 participants