Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Block API: Allow "array of attributes to be compared" for isActive property in block variations #30913
Block API: Allow "array of attributes to be compared" for isActive property in block variations #30913
Changes from 12 commits
e732e8a
a6ba309
456263c
2f84704
dc2fb96
0ed0178
9210ab4
c5fcb45
bad88e0
f82dc48
bcc9124
e836034
66af876
637b62b
66c4201
146d392
0e2c363
1ca52fc
21328a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Optional: Would it be worth validating this earlier during block registration? It might be nice for folks to know that something isn't quite right when developing blocks.
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.
Actually that's what I had in mind too in my first comment. This way we will avoid calling this check so many times.
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.
Good point! First iteration is here: 637b62b
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 think we'll still want to return false if either of these values is undefined. Blocks and variations may have optional attributes.
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 can't come up with a case for this 🤔 It seems to be working as expected:
undefined
, then that's correct and should match.variation.attributes[ attribute ] = undefined
andattributes[ attribute ] != undefined
then it won't matchvariation.attributes[ attribute ] != undefined
andattributes[ attribute ] = undefined
then it won't matchThere 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.
@gwwar can you expand on the use case you have in mind with an example?
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.
Sorry, I missed 👀 that we were already iterating over variations instead of all possible block attributes, so only the left side may be undefined, eg attributes[ attribute ]. Code here is fine 👍
btw I personally find that code reuse in tests isn't that useful. What's nicer for me is being able to read a single test case without needing to skip to different definitions. (Fixtures/snapshots can also help when things go crazy). Curious if folks had thoughts on that.
Here's a free test case, where I verified this:
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.
Yeah, I had that in mind too. 😄 That's why the last tests are totally separate. Definitely needs some cleaning. A little bit of reuse doesn't hurt though 😄
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.
Certainly, I wanted to leave a note since I was having a bit of trouble reading what the state shape was without inspecting tests.