-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add wrapper in Query block and align
support
#30804
Conversation
Size Change: +44 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
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.
Code looks good to me here.
@@ -0,0 +1,3 @@ | |||
<!-- wp:query {"query":{"perPage":null,"pages":0,"offset":0,"postType":"post","categoryIds":[],"tagIds":[],"order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":true},"layout":{"type":"list"}} --> |
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.
As long as default value for attributes is used, it should look rather like:
<!-- wp:query -->
<div class="wp-block-query"></div>
<!-- /wp:query -->
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'll investigate. I'm wondering if it has to do with these properties being objects
that is not so common in core blocks. For example queryId
is not here 🤔
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 wouldn't be surprised. I think you know already my opinion about using complex key/value-based objects as attributes. I would avoid them in the first place, so if that creates an issue, we could use the opportunity that deprecation is inevitable to refactor that as well. Anyway, I hope that is something else and you don't have to end up with a nuclear option 🤣
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 some cases (Query being the main case that comes in my mind) it makes sense to use objects
and we could always explore adding better support for them. Anyway..
How do you feel about this:494aa75
We could always use isEqual
but felt like we could avoid the extra call for most cases that is not needed (not type object
.
attributeSchema.default === value | ||
( attributeSchema.default === value || | ||
( attributeSchema.type === 'object' && | ||
isEqual( attributeSchema.default, value ) ) ) |
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.
A similar change has been proposed in a PR at some point and was rejected (don't remember why, maybe performance). It would be good to try to find this PR and the related discussions :)
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 this the one you had in mind: #26162? It's a bit different but has to do with object
default values.
I don't think performance is an issue here, since the only core block that has default object
props is just the Query
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.
This one I think #27348
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.
Isn't it a breaking change? It should be covered with a unit test if we decide to take this approach. What should we do about arrays?
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.
Of course I'll add tests if this will be the way to go. I think we could also handle arrays
the same way but first only perform the simple equality value check to avoid the extra call for most cases (performance).
I'm not sure how it's a breaking change. It will maybe affect some third party blocks by removing the default object from its comment delimiters. The default value will still be used in block manipulation in the editor, no?
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'm not entirely sure how this change relates to this PR and whether it's mandatory but I personally don't have big objections here if it's mandatory, it seems the discussions on the linked PR were mostly about performance impact.
Performance was one concern, but the other was the semantic distinction between no value and empty value (i.e. undefined !== {}
).
But yeah, in this case I'm not sure how it relates to the current PR — what am I missing?
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.
It all started from the deprecation snapshot here: #30804 (comment).
If we are okay to leave the snapshot as it was with the serialized query
object, I can remove this change and move forward this PR with the tests fixing/check.
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 the link, I see it now. I don't think this is an easy question. We'd have to weigh the trade-offs:
- Using
isEqual
in the serialiser would simplify the case of attributes with deep-valued defaults, which would in a sense "fix" how default values. - On the other hand, the serialiser has worked like this for four years now (and two in Core), and the fix might cause unexpected changes in third-party blocks who have come to rely on this quirk.
- I feel less strongly today about what I said in December in Fix an issue with the serializer not ignoring attribute values of object attribute type when the value is equal to the default. #27348 (comment), but there is still a point to consider there.
- Finally, obviously, there's the question of performance associated with deep comparison. I don't expect this to be a major factor, in practice, but we would have to test.
I'm in favour of keeping the focus of this PR on the alignment win, and leaving the serialisation matter aside.
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.
On the other hand, the serialiser has worked like this for four years now (and two in Core), and the fix might cause unexpected changes in third-party blocks who have come to rely on this quirk.
That is my main concern with this proposal. Another thing I worry about is that when comparing arrays it's unclear if the order of items should be taken into account. I guess, it's better to leave it unchanged and just open a follow-up issue to document better the current behavior in the documentation.
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.
On the other hand, the serialiser has worked like this for four years now (and two in Core), and the fix might cause unexpected changes in third-party blocks who have come to rely on this quirk.
I guess, it's better to leave it unchanged and just open a follow-up issue to document better the current behavior in the documentation.
I agree. This is a totally different and bigger issue that needs resolving on its own. I'll revert the related change.
2fbd5d9
to
aed8748
Compare
aed8748
to
46f147d
Compare
60a204b
to
02e89a6
Compare
Major kudos to @david-szabo97 for identifying the root cause of the failing test that were caused by His second fix of The same problem occurred here:#30045 (comment) when trying to bump tt1-blocks to version Puppeteer reference: https://pptr.dev/#?product=Puppeteer&version=v9.0.0&show=api-pageevaluatepagefunction-args |
5ba9a10
to
03d937b
Compare
03d937b
to
4320a20
Compare
* Block themes: Update templates to include a Query block wrapper (WordPress/gutenberg#30804) Co-authored-by: Maggie Cabrera <[email protected]>
Description
Resolves: #27589
This PR adds a wrapper
div
inQuery
block. This allows us to enablealign
support and later define semantic tags for the block. I have also addedalign
support forQueryPagination
block.Related: #30506
How has this been tested?
Checklist:
*.native.js
files for terms that need renaming or removal).