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(KvPaymentSelect): emit error instead of using broken local toast #319

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

emuvente
Copy link
Collaborator

Ran into errors of errorToast being undefined. To fix, I switched to emitting the error message so that consuming apps can display the error (or not) in whatever way they want.

@emuvente emuvente requested review from christian14b and a team November 21, 2023 22:00
Copy link
Collaborator

@mcstover mcstover left a comment

Choose a reason for hiding this comment

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

I think this is fine. We may also want to consider moving this component out to kv-components instead to reduce the need for special configs to resolve components out of the kv-shop dist folder. I made progress getting KvPaymentSelect to resolve in our CPS tests but it just leads to a different issue.

@emuvente
Copy link
Collaborator Author

We may also want to consider moving this component out to kv-components instead to reduce the need for special configs to resolve components out of the kv-shop dist folder.

KvPaymentSelect depends on the useBraintreeDropIn composable, which much of the rest of kv-shop also uses. That means moving KvPaymentSelect to kv-components would require adding kv-shop as a dependency to kv-components. That didn't feel like the right order of dependencies to me.

I made progress getting KvPaymentSelect to resolve in our CPS tests but it just leads to a different issue.

I would be curious to know what the different issues you ran into after the tests passed. I think the solution to those kind of issues should be resolved with adjusting the publishing process or adjusting the consumer build process, rather than moving components around.

@emuvente emuvente marked this pull request as ready for review November 21, 2023 22:33
@emuvente emuvente merged commit 6376b4b into main Nov 21, 2023
2 checks passed
@emuvente emuvente deleted the payment-error-message branch November 21, 2023 22:34
@mcstover
Copy link
Collaborator

mcstover commented Nov 21, 2023

@emuvente I didn't think about that deeper dependency there. Basically once, KvPaymentSelect started to resovle properly, by specifically adding concessions for @kiva/kv-shop/dist the other issues started popping up.

I'm going to separate some of the fixes I've got in place with the CPS tests and push that so we can dig into the new failures afterward.

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.

3 participants