-
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
components/utils/font: refactor away from lodash .get
#48629
Conversation
.get
Size Change: +3 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Flaky tests detected in d4b9bca. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4303096173
|
@@ -14,5 +9,5 @@ import FONT from './font-values'; | |||
* @return {string} Font rule value | |||
*/ | |||
export function font( value ) { | |||
return get( FONT, value, '' ); | |||
return FONT[ value ] ?? ''; |
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.
Unfortunately, this won't work; value
could contain dots that define a nested path, for example:
font-size: ${ font( 'helpText.fontSize' ) }; |
We'll need to parse and account for those when retrieving the field value. You could borrow the getValueFromObjectPath()
helper from #48491 which does essentially that.
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 don't think it's a path. The keys of the FONT
map contain dots, too. The JSDoc mentions its expecting one of those keys. Think we're good in this case.
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.
Glad this was a false alarm. Thanks for double-checking. I've confirmed that you're correct. 👍
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.
LGTM, thanks @flootr 🚀
What?
This PR removes Lodash's
_.get()
from thefont
helper utility in the components package.Why?
Lodash is known to unnecessarily inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality. See these for more information and rationale:
@wordpress/api-fetch
package haslodash
as a dependency #39495How?
We're using nullish coalescing instead.
Testing Instructions
npm run storybook:dev
) and inspect all the stories of theInputControl
component. Make sure it looks exactly the same as here: InputControl (Storybook).