-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix/issue 2833 - Display a notice to use 9 digit zip code for more accurate tax calculation #2835
base: trunk
Are you sure you want to change the base?
Conversation
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.
Everything is looking good and works on both classic/blocks cart/checkout already. I've just left some questions/comments for potential improvements.
* | ||
* @param string|array $message Error message. | ||
*/ | ||
public function _notice( $message ) { |
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.
Is there a reason for the function _
prefix?
* @param string|array $message Error message. | ||
*/ | ||
public function _notice( $message ) { | ||
$formatted_message = is_scalar( $message ) ? $message : wp_json_encode( $message ); |
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.
Do we want to accept scalar values for the notice message? Or should we only be accepting strings?
client/checkout-notices/index.js
Outdated
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.
Could you please review how we are using the core/notices
store and the wc/checkout/shipping-address
notice context to fine tune where we are adding the notice and see if we can take the same approach here instead of creating a more custom kind of notice?
Description
Displaying an error notice to let the user know that 9 digit zip code is recommended for more accurate tax calculation.
Related issue(s)
Fixes #2833
Steps to reproduce & screenshots/GIFs
Checklist
changelog.txt
entry addedreadme.txt
entry added