Add Slot/Fill to discounts area in cart sidebar #4248
Conversation
opr
added
category: extensibility
Type this properly when validation context is typedType this properly when validation context is typed https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/8bff20176a15baceeed0547b1bd727e1c2446c35/packages/checkout/text-input/validated-text-input.tsx#L36-L47🚀 This comment was generated by the automations bot based on a
|
|
Size Change: +1.09 kB (0%) Total Size: 987 kB
ℹ️ View Unchanged
|
| const discountsSlotFillProps = { | ||
| extensions, | ||
| cart, | ||
| contexts: { | ||
| validation: useValidationContext(), | ||
| storeCart: useStoreCart(), | ||
| }, | ||
| components: { | ||
| ValidationInputError, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Instead of doing this which is a side-effect of exporting ValidatedTextInput from the checkout package (which wouldn't have access to the same validation context instance). Could you avoid removing the encapsulation of the validation logic in ValidatedTextInput and have the component passed through on the slot-fill here? So you'd end up with something like:
| const discountsSlotFillProps = { | |
| extensions, | |
| cart, | |
| contexts: { | |
| validation: useValidationContext(), | |
| storeCart: useStoreCart(), | |
| }, | |
| components: { | |
| ValidationInputError, | |
| }, | |
| }; | |
| const discountsSlotFillProps = { | |
| extensions, | |
| cart, | |
| contexts: { | |
| storeCart: useStoreCart(), | |
| }, | |
| components: { | |
| ValidatedTextInput | |
| }, | |
| }; |
The concern I have with the current implementation is it exposes more of the implementation details to extensions and makes it harder for us to make changes in the future.
There was a problem hiding this comment.
I also have a high level of concern around exposing the entire cart implementation details here. I realize points and rewards redemption requires some ability to update the cart but this again exposes implementation details that makes it harder for us to modify how this works in the future - especially when in the current implementation there's some pretty high dependency on making sure the right shape for the cart data is fed to the receiveCart dispatcher.
This is an interface that would be fairly hard for extensions to understand and apply correctly without deep knowledge of internals.
I don't have any immediate alternatives to suggest but I'd be wary about shipping with the current implementation.
There was a problem hiding this comment.
Thanks Darren, could you clarify what you mean about the ValidatedTextInput change here? This component renders a ValidationInputError which is (at the moment) passed to ValidatedTextInput as a prop. Previously it was imported from the base-context package, but since moving the input to the checkout package, the context instance is different one, so errors don't show up. Are you suggesting to move ValidatedTextInput back out of the checkout package and instead pass it through the slot?
Also, right now I no longer need the useStoreCart context, following the addition of https://github.com/woocommerce/woocommerce-gutenberg-products-block/pull/4248/files#diff-e9030d5520892c9926d801219c89bbc21b4863a510ce18409f74b0a148f1270eR1-R24, BUT, the caveat is, after updating the cart, the extension needs to request fresh data to be loaded, which has a pretty poor UX especially on slower devices or connections: https://www.loom.com/share/8a9871aeb0b5494a84cfc54f84c00dfd
There was a problem hiding this comment.
Are you suggesting to move ValidatedTextInput back out of the checkout package and instead pass it through the slot?
Yes this. That would also remove the need to expose implementation details from the validation context right?
There was a problem hiding this comment.
BUT, the caveat is, after updating the cart, the extension needs to request fresh data to be loaded, which has a pretty poor UX especially on slower devices or connections: https://www.loom.com/share/8a9871aeb0b5494a84cfc54f84c00dfd
Ya that's not the best UX. I wonder if instead of doing this, we could provide some API that signals the checkout should do some sort of "update_cart" post to the server on the StoreAPI that extensions could react to server side and make any necessary updates? So it'd work similarly to when quantity is updated etc except extensions are simply triggering the post and response? So the rough flow would be something like:
- Extension updates a value attached to it's state in the cart store.
- Extension uses some way of signaling it's state needs updated in on the server.
- This triggers a checkout "update" POST.
- Extension is hooked into the StoreAPI "update" POST for capturing the data it needs to process server side and preparing the response.
- StoreAPI returns the updated Cart response (and the extension could react to value changes in it's cart store state)?
There was a problem hiding this comment.
Thanks for your feedback @nerrad, as we have discussed in our 1:1 and in this PR, this is the approach I think we'll continue with.
I have reverted the changes in #4238 and instead opted to pass this through the slot, due to us not wanting to pass validation contexts through the slot.
This PR is now ready for a full review 🎉
opr
force-pushed
the
feature/coupon-area-slotfill
branch
from
b6a4a74
to
d0f49cc
Compare
There was a problem hiding this comment.
Changes look good. I left a couple of CSS suggestions but I'm approving because none is blocking.
One thing I noticed is that when I run npm run build in Points and Rewards, I can't see the rewards panel, it only works when I run npm run start. Is that a known issue?
Another thing is that text doesn't seem to wrap correctly:
assets/js/base/components/cart-checkout/totals/coupon/style.scss
Outdated
Show resolved
Hide resolved
|
Oh, one more thing: are the up and down arrows expected in the number input? They collide with the placeholder. I would personally remove them, but leaving the decision to you:
|
| components: { | ||
| ValidatedTextInput, | ||
| }, |
There was a problem hiding this comment.
I'm still not sold on this components pattern tbh. Why are we doing it this way? Do people need this exact input with its validation context and all? can't just they get a regular input and handle validation themselves?
There was a problem hiding this comment.
I guess I got too bogged down here, I think there's actually no reason to pass this component through... let me get rid of it and see how it goes 👌🏼 .
There was a problem hiding this comment.
Just noting that Thomas and I chatted about this today. A couple reasons why this was passed through was to:
- Expose (what would likely be a common type of) component for extension developers to use to preserve consistent styling and behaviour.
- Make it easier for extension developers to implement this kind of functionality.
However, the tradeoffs are that we are establishing a contract to preserve this component and kicks off a pattern that might be hard to maintain in the future if extension developers have need for other types of components in the checkout (checkbox etc). There's some element here of just including this component because Points & Rewards needs it.
So for now, we agreed that we'll hold off exposing this and Thomas will implement the functionality directly in Points & Rewards. I do think we'll still need to address the issue of exposing some components/style system for extension developers to use so they can inherit/preserve the current (and any evolution of the) design of the blocks which makes for a better shopper experience. The work Thomas will be doing in Points and Rewards implementation will unblock that integration but will be more inherently fragile in relation to potential changes in the Cart and Checkout design down the road (for example when we potentially start including Global Styles from Gutenberg to allow for more customization options by merchants etc). So I think we will need to address this in a followup.
There was a problem hiding this comment.
Expose (what would likely be a common type of) component for extension developers to use to preserve consistent styling and behaviour.
Make it easier for extension developers to implement this kind of functionality.
Why wouldn't having the component in @woocommerce/blocks-checkout be enough? previously we passed shippingSelector from there to preserve an already passed context I guess, but that pattern is up for reconsideration during this cooldown in which we might delete the shipping methods slotFill for a registration API.
In my mind, components was used to pass components that are harder to move to the checkout package or are very opinionated to a specific case.
I'm not against exposing components in general via a components package, in that case, we have a real boundary of "all code in this folder is public" type of protection, making it easier for us to maintain it.
There was a problem hiding this comment.
This is exactly the sort of discussion that we need to work through separately now that we are starting to see integrations that could use more of these components (whether published via package, or passed through as an interface prop etc).
There was a problem hiding this comment.
I already tried moving the ValidatedTextInput component to the checkout package, but it didn't work with the validation context, because they were in different packages it seemed they couldn't work together. Then we tried passing the context through the slot, and that worked, but wasn't the right approach to take as we didn't want to expose this context to third parties.
For now I'm sticking with what Darren has already said - we will duplicate the component in P&R until we can revisit this and find a way to expose ValidatedTextInput to extensions.
This is so it doesn't need to get them from useValidationContext.
This will be used to control additional props on the input element of TextInput
opr
force-pushed
the
feature/coupon-area-slotfill
branch
from
a08709f
to
d7bbe0f
Compare
There was a problem hiding this comment.
Thanks for the updating this PR @opr! Changes look good to me. I noticed there is an issue in Firefox (input field lets you write numbers and on unfocus the label collides with the input):
I would say we don't need to fix this since this is a Firefox bug and IMO an edge-case most users will not encounter. But wanted to raise it in here in case you think differently.
I will leave the final approval of this PR to @senadir given that he made other comments.
Yep, you can also write the letter |
This PR adds a Slot/Fill to the Cart sidebar (The same functionality is required in Checkout, but this will come in another PR). Extensions can register components to be fill this Slot, it is rendered just underneath the
TotalsCouponcomponent.The PR also includes a (partial) revert of #4238 (specifically, the part where
ValidatedTextInputwas moved to the checkout package) but keeps the part where styles, and a spread were added toValidatedTextInputto handle the cases when this component is used astype="number".packages/checkout/utils/update-cart-from-api.ts- these are changed again in #4298 which is based off this branch.Fixes #4205
How to test the changes in this Pull Request:
feature/add-inputs-for-points-redemptionfrom https://github.com/woocommerce/woocommerce-points-and-rewards/ and ensure you've runnpm run buildin that project.includes/class-wc-points-rewards-extend-store-endpoint.phpin Points and Rewards and comment out the following lines (66-72):Changelog