Add 'Order notes' to Checkout block #2877
Conversation
Aljullu
added
status: needs review
type: enhancement
| @@ -26,6 +26,7 @@ export const DEFAULT_STATE = { | |||
| hasError: false, | |||
| calculatingCount: 0, | |||
| orderId: checkoutData.order_id, | |||
| orderNotes: '', | |||
There was a problem hiding this comment.
I added the orderNotes value in the Checkout context. It doesn't belong to the Shipping data neither the Billing data, so adding it to those contexts felt wrong. At the same time, creating a specific context to only hold the order notes seemed a bit overwhelming. That's why I decided to use the Checkout context to store it, but I'm not 100% sure about it either.
There was a problem hiding this comment.
This is consistent with the approach I took for createAccount - makes sense to me.
There was a problem hiding this comment.
I think adding this to the checkout-state context was a good call here. One thing to remember is that checkout-state is a child of the Cart context provider too.
|
Size Change: +2.21 kB (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is looking good and testing well. I tested with Chrome, Firefox, and Safari on macOS and Safari on iOS and saw no issues.
I've left some code polish comments - no blockers IMO, up to you if you think there's anything to address there.
UX wise I have a couple of thoughts, none are particularly critical - again, up to you if you want to follow up or merge as is.
- The note text is lost when if the shopper unticks the box. It would be nice if the text they enter is preserved when check/uncheck the box. Also, the text is lost if you navigate away - would be cool if this persisted somewhere, similar to how your address info is (sometimes) persisted.
- Grouping / naming of the editor option. This is pretty minor, and definitely shippable as is. Just wanting to raise it to avoid having more single-item option groups appear in the editor sidebar over time.
| ) => { | ||
| let newState; | ||
| let newState = state; |
There was a problem hiding this comment.
With the changes in 66f3d33, for example, it's not always sure we will assign a value to newState, so it's good to initialize it to the previous state to avoid unexpected errors.
| @@ -35,22 +35,34 @@ const FormStep = ( { | |||
| description, | |||
| children, | |||
| disabled = false, | |||
| showCounter = false, | |||
There was a problem hiding this comment.
Is this a new prop for showing the step number? Seems handy. Should we add storybook or tests to verify this and clarify the interface?
A tweak to the name might be clearer IMO - e.g. showStepNumber or showStepIndex. Also I think the default should be true – I'm guessing that "hide" is the less common use.
There was a problem hiding this comment.
Could also export two variations, NumberedFormStep or FormStep.
I wonder what happens when there are hidden-numbered steps in between numbered steps - would that mean we need control of the numbering? (Thinking ahead, not essential to consider now!)
There was a problem hiding this comment.
A tweak to the name might be clearer IMO - e.g. showStepNumber or showStepIndex. Also I think the default should be true – I'm guessing that "hide" is the less common use.
Makes a lot of sense, done in 9c3a480 and 0c8cf24.
I wonder what happens when there are hidden-numbered steps in between numbered steps - would that mean we need control of the numbering? (Thinking ahead, not essential to consider now!)
The counter is only incremented in steps that show the step number. I think this is good enough for now.
There was a problem hiding this comment.
The counter is only incremented in steps that show the step number.
Awesome!
| 'woo-gutenberg-products-block' | ||
| ) | ||
| : __( | ||
| 'Notes about your order.', |
There was a problem hiding this comment.
Is this smart message implemented in the previous / legacy checkout? If this is new behaviour, might be safer to keep it super simple for now.
There was a problem hiding this comment.
That's not implemented in the shortcode checkout, but I considered it didn't make much sense keeping the 'Notes about your order, e.g. special notes for delivery.' for orders which don't include shipping. I don't mind bringing it back, though.
@LevinMedia Checkout shortcode uses the string 'Notes about your order, e.g. special notes for delivery.' for the order notes placeholder. Does it make sense removing or changing the 'e.g. special notes for delivery' part for orders without shipping (digital products, for example)?
There was a problem hiding this comment.
Sounds like a nice new feature :)
| @@ -2,9 +2,8 @@ | |||
| top: -96px; | |||
| } | |||
|
|
|||
| .wc-block-checkout__add-note, | |||
| .wc-block-checkout__keep-updated { | |||
There was a problem hiding this comment.
Assuming these are remnants from previous markup, no longer used :)
There was a problem hiding this comment.
Yes, exactly. .wc-block-checkout__keep-updated is not used anywhere and .wc-block-checkout__add-note styles have been moved to its own file.
| @@ -148,6 +149,28 @@ const BlockSettings = ( { attributes, setAttributes } ) => { | |||
| /> | |||
| ) } | |||
| </PanelBody> | |||
| <PanelBody | |||
| title={ __( 'Order notes', 'woo-gutenberg-products-block' ) } | |||
There was a problem hiding this comment.
I see this is in a category of its own - not ideal. Keen for feedback from @LevinMedia about how to best group these options.
Maybe we could generalise the Address options group to Form options and include notes there.
Or maybe there are more options that might be grouped with order notes in future, so we could generalise the title/description.
There was a problem hiding this comment.
I see this is in a category of its own - not ideal.
Why is it not ideal? What would be ideal (I think you're suggesting generalizing the options as being the ideal?).
Form options might be a good call here given that future iterations might be adding more toggles (required postal codes for instance).
There was a problem hiding this comment.
Just that groups of one wouldn't scale well and could become overwhelming. If there are more Order notes options coming in future then the group makes sense. Not a major :)
| case SET_ORDER_NOTES: | ||
| newState = { | ||
| ...state, | ||
| orderNotes, | ||
| }; | ||
| break; |
There was a problem hiding this comment.
Does a new state need to be created every time SET_ORDER_NOTES is called? Could you compare orderNotes and only update the state if there's a change?
There was a problem hiding this comment.
I added a check to ensure the new state is only created if order notes have changed: 66f3d33.
| @@ -148,6 +149,28 @@ const BlockSettings = ( { attributes, setAttributes } ) => { | |||
| /> | |||
| ) } | |||
| </PanelBody> | |||
| <PanelBody | |||
| title={ __( 'Order notes', 'woo-gutenberg-products-block' ) } | |||
There was a problem hiding this comment.
I see this is in a category of its own - not ideal.
Why is it not ideal? What would be ideal (I think you're suggesting generalizing the options as being the ideal?).
Form options might be a good call here given that future iterations might be adding more toggles (required postal codes for instance).
Aljullu
force-pushed
the
fix/1483-checkout-order-notes
branch
from
49b7113
to
4a98c42
Compare
|
Thanks for the reviews @haszari & @nerrad.
Fixed in 668ae71. 👍 |
There was a problem hiding this comment.
The note text is lost when if the shopper unticks the box. It would be nice if the text they enter is preserved when check/uncheck the box. Also, the text is lost if you navigate away - would be cool if this persisted somewhere, similar to how your address info is (sometimes) persisted.
Fixed in 668ae71. 👍
Looking good, thanks for addressing all the minor feedback too :)
| @@ -35,22 +35,36 @@ const FormStep = ( { | |||
| description, | |||
| children, | |||
| disabled = false, | |||
| showStepNumber = false, | |||
There was a problem hiding this comment.
I see the default is still false - that seems the wrong way around to me, as most steps have the number, showStepNumber=true is the previous behaviour. Not a blocker, just checking your intention here.
There was a problem hiding this comment.
Yeah... I don't have a strong opinion on this. Changed in 7397426.
Thanks for the review.
Fixes #1483.
Screenshots
How to test the changes in this Pull Request:
Customer provided note.Changelog