Add Shuffle option to sections via pattern category #59251
Merged
+127
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
added
the
[Feature] Patterns
jorgefilipecosta
requested review from
ellatrix and
ajitbohra
as code owners
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
1 similar comment
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +757 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 049fe36. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7992197607
|
scruffian
reviewed
Feb 21, 2024
scruffian
reviewed
Feb 21, 2024
scruffian
reviewed
Feb 21, 2024
scruffian
reviewed
Feb 21, 2024
scruffian
reviewed
Feb 21, 2024
mtias
reviewed
Feb 21, 2024
| @@ -178,6 +179,7 @@ export function PrivateBlockToolbar( { | |||
| clientId={ blockClientId } | |||
| /> | |||
| ) } | |||
| <Shuffle clientId={ blockClientId } /> | |||
There was a problem hiding this comment.
I think we need to place this outside this group.
There was a problem hiding this comment.
It was updated to match this proposal.
jorgefilipecosta
force-pushed
the
add/pattern-shuffling
branch
from
df92313
to
e230bb2
Compare
scruffian
force-pushed
the
add/pattern-shuffling
branch
from
e230bb2
to
b474b15
Compare
scruffian
approved these changes
Feb 24, 2024
|
One shortcoming of this solution is that its possible to be shuffled to the same pattern again, but I'm not sure there's an easy way round that. |
jorgefilipecosta
force-pushed
the
add/pattern-shuffling
branch
from
b474b15
to
30b4959
Compare
jorgefilipecosta
requested review from
kevin940726 and
Mamaduka
as code owners
jorgefilipecosta
force-pushed
the
add/pattern-shuffling
branch
from
a318ba5
to
fd3cb9f
Compare
I'd say that's expected, and fine. Like shuffling songs on repeat all. |
jorgefilipecosta
force-pushed
the
add/pattern-shuffling
branch
from
fd3cb9f
to
803a205
Compare
I don't think that's how shuffling music works. Usually its far from random - it will be careful to avoid giving you the same thing twice in a row and often avoids giving you the same thing twice until all the other options have been played once. |
ah, I misunderstood. so this isn't cycling — it's truly random? |
|
Yes, it uses |
andrewserong
changed the title
|
The shuffling feels a bit random at the moment (trying in 2024 theme templates). I wonder what we can do to improve things. The other thing that I wanted to mention is whether we should surface the categories somewhere in the UI of the blocks. It's not entirely clear (by selecting blocks or looking at list view) why some blocks have a shuffle button and others don't. cc @richtabor @jasmussen in case you have ideas. |
Can we just cycle through them? We mainly want to avoid you seeing the same pattern twice in a row. It can still be called random, because you press your button and you won't know what you'll get.
I think this is a good callout, but I wonder if the pieces here won't fall in place on their own once we're a bit further with pattern overrides and content only pieces? I recognize the button isn't currently tied just to those properties, but there may be connective tissue between those and "patterns with categories". If nothing else, then in the top of the inspector, the description area, that seems a place to surface that this is a pattern in category n. CC: @SaxonF |
|
One more thing, can we add animation to this? I.e.
|
The issue was not the randomness but more whether the alternatives were actually relevant 😬 |
|
As part of this I think we should change the way the template part replace flow works: #59883 |
|
What are the testing instructions for this feature? |
|
Managed to test it, wasn't too hard to figure it out. 😄
I wonder if this is down to the curation of categories. The first one I tried was in the 'banners' category, and was more of a 'hero', but also had the 'text' category which seems like a bit of a catch all. I got some random things like FAQs because of that. I think it would be possible to make the shuffling also work in a more reliable way. Seems like it should be possible if the way the list is shuffled is reproducible, and for that I think it would require a seeded random number generator. The stored array of categories is the data that remains consistent, and I think that could become the seed. Next sort the list of patterns into alphabetical order, then finally shuffle them using the seeded random number generator. Or use a library like https://github.com/robbiespeed/seeded-shuffle 😄 |
| // Check if the pattern has only one top level block, | ||
| // otherwise we may shuffle to pattern that will not allow to continue shuffling. | ||
| pattern.blocks.length === 1 && | ||
| pattern.categories.some( ( category ) => { |
There was a problem hiding this comment.
Hiya! I think we have to check whether the pattern.categories is available or not. Otherwise, it will throw an error if the pattern doens't have the category.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR does the following changes:
In collaboration with @scruffian and @richtabor.
Screen recording
Screen.Recording.2024-02-21.at.17.14.05.mov