-
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
Tag Cloud: Add ability to change number of tags shown. #32201
Tag Cloud: Add ability to change number of tags shown. #32201
Conversation
Looks like I have a test to update. I'll work on that. |
bc6afd7
to
b2a2535
Compare
Rebased and updated tests. |
Apologies, not completely sure if I updated the tests properly. Or create a copy of the existing ones, to make sure that it works properly with no |
b2a2535
to
b725cf7
Compare
c2d0408
to
1853432
Compare
Okay. I decided to leave the existing test to test the default value, and add a new one, to test attributes created when a number of tags is selected. Tests updated. |
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 your work here @getsource, this looks good!
I don't think we need to create yet another fixture for this block for the new attribute and the base one would be fine to be updated. You have to update core__tag-cloud.html
file with <!-- wp:tag-cloud {"taxonomy":"category","numberOfTags":6} /-->
for example.
So in order to update the fixtures for the tests you have to edit only the html file (not the .serialized
one) and then run npm run fixtures:regenerate
.
After these minor changes this will be good to go.
Thanks much @ntsekouras! |
I think we can change the minimum to 1 and let the user decide if they want to display one most used tag. It will also match the minimum number used in other blocks. |
@Mamaduka Sounds fine to me! On one hand, it's not really a cloud with 1. I'll update this in the next pass. |
1853432
to
e1d8dbc
Compare
Okay, I updated according to the most recent changes we talked about. Hopefully I caught them all! I'll note that this PR currently lists #21951 as one of the items it will fix. It does allow raising the number, but not as high as some users would like: I'm still thinking that there should be a limit, with the reasons for the current max described in this PR's description, but figured I should mention in case we want to leave that issue open when this is merged / for a second opinion. |
Also agree with your decisions about the max value in your PR's description. |
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 and tests seem not related. I'll restart again later to have a green CI to land.
Thanks for your work and wanted to mention that your explanations for your decisions were great!
Awesome work !💯
@getsource can you rebase to get a green CI. The issues has been handled in trunk now. |
e1d8dbc
to
589cc71
Compare
Sure, thank you! Rebased, and looks to be passing now. |
Thanks much for your help and kind words! |
I've received some feedback that the hard limit of |
Description
Adds a feature to the Tag Cloud block to allow the user to choose the amount of tags shown in the cloud.
The default number of tags is currently set to 45, because this matches the default currently used in
wp_tag_cloud()
.Right now there is a minimum of 3, and max of 100 set.
The minimum of 3 was chosen to allow for the block to function as a tag cloud (as with one tag, it'd be unclear it's a tag cloud).
The max matches the one used in
latest-comments
and thelatest-posts
/query-controls
default. It also avoids potential performance issues with high numbers of tags being requested, or limitless queries.All of this open for change/discussion, of course!
Fixes: #32173
Fixes: #21951
How has this been tested?
Screenshots
Screen.Recording.2021-05-25.at.22.54.17.mov
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).