-
Notifications
You must be signed in to change notification settings - Fork 21
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
Product Block Editor: Map SelectWithTextInput
and DateTime
inputs to the custom product blocks
#2178
Conversation
…lect-with-text field.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/support-product-block-editor #2178 +/- ##
========================================================================
+ Coverage 60.8% 60.9% +0.1%
- Complexity 4151 4153 +2
========================================================================
Files 453 453
Lines 17824 17843 +19
========================================================================
+ Hits 10843 10865 +22
+ Misses 6981 6978 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Thanks @eason9487 for working on this. Overall looking very good.
I left some comments in the code and a warning in regards the functionallity.
- In block.json, it needs to list `"postType"` in the `usesContext` array to ask for the current post type ('product' or 'product_variation') from the context provider. | ||
- Forwarding the `context.postType` to `useProductEntityProp` hook is required in order to be compatible with both simple and variation product block templates. | ||
|
||
#### Derived value for initialization |
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.
❓ Can we elaborate or reword this explanation? Maybe my lack of knowledge of the concepts but not sure if I understand what those "derived values" are. I assume is just the data itself?
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.
In the React ecosystem, the derived value/state is commonly referred to the value calculated based on another value. I'm guessing that it probably comes from the wording in the React API getDerivedStateFromProps
.
Added in 0965ef9.
js/src/blocks/index.js
Outdated
import DatetimeFieldEdit from './product-date-time-field/edit'; | ||
import datetimeFieldMetadata from './product-date-time-field/block.json'; |
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.
I guess it should be DateTimeFieldEdit
and dateTimeFieldMetadata
because there is a -
between date and time in the folder name. Another option may be to call the folder product-datetime-field
.
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.
Thanks! Fixed in b94f33a.
"$schema": "https://schemas.wp.org/trunk/block.json", | ||
"apiVersion": 2, | ||
"name": "google-listings-and-ads/product-date-time-field", | ||
"title": "Product datetime control", |
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.
💅 Minor: can we keep it consistent with the block name conventions you are using?
So in this case:
"Product datetime field."
I don't mind control or field, but I think is better to call them in one single way. WDYT?
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.
Adjusted in 776ffb8.
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.
💅 WDYT of renaming value
, setValue
, setNextValue
to dateTime
, setDateTime
, setNextDateTime
So it's less generic.
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.
The use of value
and setValue
naming is to maintain consistency in the state and updater names that actually operate to the product data (wp-store) in each custom block.
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.
In the legacy Editor you can set the date without time and it saves the time as 00:00.
Screen.Recording.2023-12-15.at.15.28.51.mov
In the new editor, you cannot update til you update the time. If you edit only the date and also another field to allow the update button, the changes are not saved.
Screen.Recording.2023-12-15.at.15.24.56.mov
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.
Thanks for finding this! Adjusted in b916bd3.
"$schema": "https://schemas.wp.org/trunk/block.json", | ||
"apiVersion": 2, | ||
"name": "google-listings-and-ads/product-select-with-text-field", | ||
"title": "Product select control with text control", |
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.
💅 In line with the other block. WDYT to keep is as just Fields?
"Product Select with Text Field"
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.
Adjusted in 776ffb8.
if ( nextOptionValue === customInputValue ) { | ||
setValue( text ); | ||
} else { | ||
setValue( nextOptionValue ); | ||
} |
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.
💅 This can be reduced to:
setValue( nextOptionValue === customInputValue ? text : nextOptionValue );
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.
I lead toward keeping it as it is.
|
||
const FALLBACK_VALUE = ''; | ||
|
||
export default function Edit( { attributes, context } ) { |
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.
💅 Can we add a bit of documentation of what customInputValue
attribute is for?
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.
Added JSDoc for all custom blocks by d8252b8.
…tring and fall back to '00:00:00'. Address: #2178 (comment)
@puntope, thanks for the code review. This PR is ready for the new round. Could you take another look? |
…block-editor-custom-select-with-text-and-date-time
async function resolveValidationMessage( inputRef ) { | ||
const input = inputRef.current; | ||
|
||
if ( ! input.validity.valid ) { | ||
return input.validationMessage; | ||
} | ||
} |
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.
Why do we need validation for the datetime? I wasn't able to set an invalid date time in the inputs.
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.
I didn't test all browsers, but Chrome and FireFox both allow entering date by keyboard, which may result in an invalid value.
Kapture.2024-01-03.at.11.23.23.mp4
"請輸入有效的日期" and "請輸入有效的時間" mean "Please enter a valid date" and "Please enter a valid time".
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.
✅ LGTM Thanks for the changes
I left tho a question in regards the validation.
d50c151
into
feature/support-product-block-editor
Changes proposed in this Pull Request:
To be compatible with Product Block Editor, this PR makes the conversions for the last two attribute inputs:
SelectWithTextInput
andDateTime
inputs to the above custom product blocks.AttributesBlock
class to 8.4 as it's the minimum version that the__experimentalTextControl
component is exported from@woocommerce/product-editor
.Screenshots:
📷 Field is converted from text block to select-with-text block
📷 Date-time block
Detailed test instructions:
📌 Prepare test environment
npm install
npm start
📌 Test the select-with-text block
return true;
google-listings-and-ads/src/Integration/YoastWooCommerceSeo.php
Lines 32 to 34 in abd3492
📌 Test the date-time block
Additional details:
TODOs will be cleaned up in a separate PR.
google-listings-and-ads/src/Admin/Input/Input.php
Lines 60 to 61 in abd3492
google-listings-and-ads/src/Admin/Product/Attributes/AttributesBlock.php
Lines 194 to 197 in abd3492
Changelog entry