Add shipping class section and dropdown #34684
Conversation
github-actions
bot
added
focus: react admin
package: @woocommerce/data
Test Results SummaryCommit SHA: c6405f1
To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
There was a problem hiding this comment.
Overall nice work on the @mdperez86, you put this together quickly and well :)
I did encounter a couple issues, as my shipping classes never showed up in the list, which isn't a problem with your code, just the shipping class data store, but I left a suggestion around that for now. - this was a build issue
The shipping classes weren't saving correctly, as we want to use shipping_class versus shipping_class_id. shipping_class uses the slug.
I left some other comments and suggestions within the code.
| <SelectControl | ||
| label={ __( 'Shipping class', 'woocommerce' ) } | ||
| { ...getTextControlProps( | ||
| getInputProps( 'shipping_class_id' ) |
There was a problem hiding this comment.
shipping_class_id is a readonly field, it looks like we want to use shipping_class instead and use the slug as the value instead of the id.
See https://woocommerce.github.io/woocommerce-rest-api-docs/#product-properties for more context on the readonly field.
There was a problem hiding this comment.
Ohh!!!, awesome @louwie17 that was a question I had and you answered. So are we gonna use shipping_class (slug) as the options value and also to send it when the product gets save right?
There was a problem hiding this comment.
So are we gonna use shipping_class (slug) as the options value and also to send it when the product gets save right?
Yes, that is right!
| return [ | ||
| ...DEFAULT_SHIPPING_CLASS_OPTIONS, | ||
| ...mapShippingClassToSelectOption( shippingClasses ), | ||
| ]; |
There was a problem hiding this comment.
We tend to avoid creating any new arrays or objects within the useSelect function. As everything returned by the useSelect gets "cached". Meaning the useSelect will only trigger a React re-render when the returned value is different.
It only checks this by ===, so any newly created objects or arrays will always show as dirty as the reference will be different.
In theory this is fine for your array as long as the order stays the same, as the useSelect checks for each value with the object returned.
In this case it would loop over each shipping class item and check if it matches the old one.
What I would recommend is just returning the shippingClasses:
return {
shippingClasses: getProductShippingClasses< ProductShippingClass[] >();
}
Then outside of the useSelect append the two arrays and map them.
Hope that makes sense, let me know if you have any questions?
| EXPERIMENTAL_PRODUCT_SHIPPING_CLASSES_STORE_NAME | ||
| ); | ||
| const shippingClasses = | ||
| getProductShippingClasses< ProductShippingClass[] >(); |
There was a problem hiding this comment.
It appears there might be something broken with the crud data store at the moment and the above returns null all the time (at-least for me). This might of been something I broke :/ which I thought I had fixed.
To get around it for now you could pass an empty object query object, but I will get a fix up for this.
There was a problem hiding this comment.
Actually this might of been an issue with my build, I will get back to you on this.
There was a problem hiding this comment.
Yup, just double checked and made sure my build was correct. This is indeed working correctly as is, sorry for the confusion.
| /** | ||
| * External dependencies | ||
| */ | ||
| import { SelectControl } from '@wordpress/components'; |
There was a problem hiding this comment.
We may want to use the new experimental SelectControl from @woocommerce/components for this so the dropdown supports typeahead field.
I am not sure how long a shipping class list could get, but being able to search for them in the field would be really nice.
cc: @jarekmorawski on this if we should use a standard dropdown for this, or a typeahead field?
There was a problem hiding this comment.
Because SelectControl is still experimental I will continue using the WP one until this SelectControl will become more stable. I left this branch with a partial implemetation that we can reuse for autocomplete implementation.
|
|
||
| const DEFAULT_SHIPPING_CLASS_OPTIONS: SelectControl.Option[] = [ | ||
| { value: '', label: __( 'No shipping class', 'woocommerce' ) }, | ||
| { value: '-1', label: __( 'Standard shipping', 'woocommerce' ) }, |
There was a problem hiding this comment.
Thanks for leaving a question surrounding this in the original issue :)
There was a problem hiding this comment.
According to this comment we will have only the 'No shipping class' option and it will be the default.
mdperez86
force-pushed
the
add/34328-shipping-shipping-class-dropdown
branch
from
edbeec5 to
8f6789c
Compare
github-actions
bot
added
the
release: highlight
mdperez86
force-pushed
the
add/34328-shipping-shipping-class-dropdown
branch
2 times, most recently
from
c4fc857 to
e816f21
Compare
mdperez86
force-pushed
the
add/34328-shipping-shipping-class-dropdown
branch
from
e816f21 to
08a73b3
Compare
github-actions
bot
removed
the
release: highlight
louwie17
left a comment
louwie17
left a comment
There was a problem hiding this comment.
@mdperez86 thanks for the updates, this is testing very well, and updates work great.
I only noticed one minor issue, given the getProductShippingClasses is async, I briefly see "No shipping class" when I open an existing product even though I have a shipping class selected.
Kapture.2022-09-21.at.17.36.21.mp4
I wonder if we could maybe just show a loading icon for the entire block first, kinda like this wireframe: 5sAIeTRd9Yp7nSCT33BAWz-fi-6823%3A312388
Or show it as blank until the shippingClasses are retrieved, or show a loading icon within the dropdown if that is possible.
But following the wireframe apparoach is probably the best:
louwie17
left a comment
louwie17
left a comment
There was a problem hiding this comment.
Thanks for all your hard work and doing the several updates. This is testing very well and looks great, nice work on the loading screen as well :)
|
Hi @mdperez86, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
* Add shipping class section and dropdown * Add comment suggestions * Add a Spinner during shipping classes resolution
All Submissions:
Changes proposed in this Pull Request:
Closes #34328.
How to test the changes in this Pull Request:
WooCommerce > Settings > Shipping > Shipping classesadd it.
4. A Shipping class dropdown should list `No shipping class` option and the other shipping classes you added in point 1.
5. When click on helper text `global settings` below the dropdown you should be redirected to `WooCommerce > Settings > Shipping > Shipping classes` page in a new tab
6. Add a Spinner during shipping classes resolutionScreen.Recording.2022-09-22.at.1.37.03.PM.mov