-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Update: Font Size Picker component documentation #11404
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
Update: Font Size Picker component documentation #11404
Conversation
| onChange, | ||
| value, | ||
| withSlider, | ||
| withSlider = false, |
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 supposed to be part of this PR?
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.
Hi @ZebulanStanphill,
By default withSlider was already "falsy" but not explicitly. If we add an explicit false value, we can document that it is false by default. I think documenting this is helpful for the users, that is the reason this change was added here.
chrisvanpatten
left a comment
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.
Don't want to block merging because my tweaks are minor and perfect is the enemy of good, but I have a few suggestions. Thanks for getting this documented 👍 🥇
| ### fontSizes | ||
|
|
||
| The current font size value. If a button value matches the font size value that button is pressed. RangeControl is rendered with this value. | ||
| An array of font size objects. The object should contain properties size and name. |
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.
Do you also need to provide a "slug"? I might be wrong but I think it's expected in terms of generating CSS classes (although maybe that doesn't get passed into this version of the component?)
I'd also like to see an example here… happy to add that, if you want. Something simple like
{
size: 12,
name: "My Font Size",
slug: "font-slug" // if needed
}
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 know we show it above, but being more explicit will be good for users who are newer to JS.)
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.
You are totally right I missed the slug property and it is used to generate classes in the component. The situation was resolved. thank you for catching 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.
@jorgefilipecosta No prob! I also thought that it was possible we only used slug inside the editor version of this component so I'm glad I checked :)
48749cc to
968b046
Compare
968b046 to
3facd94
Compare
chrisvanpatten
left a comment
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 is very very good, happy to see it land!
|
Thank you for your review @chrisvanpatten! |
Description
This pr updates the documentation of @wordpress/components/FontSizePicker.
The component was updated with additional props added and the structure of existing props changed but the updates to the docs were missing.
How has this been tested?
Just documentation updates. I checked the sample code works and does not contain linting error, by pasting it in an existing block.