-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 NumberControl
to support min/max font size for Tag Cloud Block
#37311
Add NumberControl
to support min/max font size for Tag Cloud Block
#37311
Conversation
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 @amustaque97! 💯
This works well code wise! I left a couple of notes about making the controls a bit more compact and it would be ready to land. After the changes we would just need a design approval of course maybe by @jasmussen?
You need to add the changes to block docs since you changed block attributes. Run |
Thank you for the PR! Here's how the tag cloud looks in trunk: Here's this PR applied: I thought about whether there was a better way to combine min/max font sizes into a singular control, but for comparison the paragraph font size looks like this: So it's probably best to not diverge too far from this. The following is a mockup that uses new components from the effort outlined in #27331, so it looks a bit different from what's shipping but it's what it could eventually look like: But one single recommedation I'd make is to adopt the unit-selector input field. That would both let us remove the In smaller changes, if it isn't too much, I'd change the panel title from "Tag Cloud settings" to just "Settings" — there really isn't need for the redundancy. Nice work! |
Thank you for the reply! I can change the units from |
Thanks for the iterations here @amustaque97! In order to implement properly what @jasmussen suggests you should use
We can't change the default behavior to This is a bit more extra work but provides more flexibility. |
What occurred to me right now is that with unit control we can set different units per size (smallest, largest), but there is no support for this in Maybe a first iteration of this could be to support only the |
@ntsekouras Again! this was too quick 😅 💨 . TBH I was not ready for the review yet. I was trying few things. Best thing to do right now is to wait for the reply from design team. |
I would suggest is to make both Adding a link for sample example though it is not an actual implementation but the idea would be same. |
This looks great to me! Thanks for the extra effort. I don't think we need to sync the two unit types, it adds only limited value and I could imagine in the future someone wanting a minimum font size in pixels, and a max font size in responsive Honestly this one is good to go pending just a code sanity check. Is there a reason we limit support to |
|
Oh that's helpful context. 🤔 The unit selector you've implemented, though, is useful, and it would be nice to keep it. How hard would it be to keep those two in sync after all? |
In order to do that I guess we would just handle this on Noting that changing the two attributes(smallest, largest) into type |
Addressed review comments. Code wise it is functional but on UI it doesn't update when we update font unit. I wanted to use something like unit so that it will be easy to update UI if we are changing units at code level. Looking forward to your inputs Nik! |
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 iterating on this one once again @amustaque97! Great work!
I've left some comments, but it's getting really close!
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 iterations here @amustaque97!
The other thing that sometimes doesn't update the unit
in the control, is something that seems to be internal to UnitControl
. The values are updated properly in the block and when the controls are rerendered everything seems fine. --cc @ciampo for this issue.
I can reproduce sometimes when changing one font size and then change the unit of the other fontSize control.
Screen.Recording.2022-02-25.at.12.51.05.PM.mov
Hey @ntsekouras! Out of curiosity, are you able to reproduce that bug also in Storybook, or is it just happening in the context of the Tag Clouds settings? |
If you add another |
test cases were failing because of new attribute `fontSizeUnit`. It is fixed now.
given a comment to handle decimal value case when passed to `unit` property. I updated the regex to `/^\d*\.?\d*/`. This regex can parse values like `123.3px` or `.33px` or `123px`.
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Fix formatting
06437ea
to
db24fd3
Compare
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 all the iterations and your patience here @amustaque97! Great work! 💯
I've left a minor change to the min value, but tested and this looks good! Attribute sanitization looks good too.
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
Fixes: #37166
Description
Add
NumberControl
input to enter minimum and maximum font size value. It will help user to decide how they want tag cloud to look. Earlier it used to modify fontsize based on the number of post for a particular tag.NOTE: When I reload the page these values reset to default values. 😞
How has this been tested?
http://localhost:8888/wp-admin/edit-tags.php?taxonomy=post_tag
/tag cloud
block./tag cloud
block go to settings, there you can see 2 new input has been added to control font size./tag cloud
block and check its font size.Screenshots
NumberControl
After post publish
Types of changes
Feature
Checklist:
*.native.js
files for terms that need renaming or removal).