-
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
Lodash: Refactor away from _.kebabCase()
in EditorHelpTopics
#48776
Conversation
Size Change: +375 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
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, added a comment. Thanks, @tyxla!
@@ -1,7 +1,7 @@ | |||
/** | |||
* External dependencies | |||
*/ | |||
import { kebabCase } from 'lodash'; | |||
import { paramCase as kebabCase } from 'change-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.
Can any of these uses (labels, as far as I can tell) be non-ASCII? I remember this caused issues before.
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.
Flaky tests detected in fc78d13. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4343542072
|
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.
Sounds good, thank you @tyxla!
Minor nit; feel free to ignore: perhaps we could simplify things by doing:
import { paramCase } from 'change-case';
const kebabCaseSettings = [...];
function kebabCase( str ) {
return paramCase( str, kebabCaseSettings );
}
?
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. I found no issues while testing the changes with an iPhone 14 simulator and Google Pixel 3a emulator. Thanks for the ping.
Thanks a bunch for the confirmation @dcalhoun 🙌 |
What?
This PR removes Lodash's
_.kebabCase()
from theEditorHelpTopics
component.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
paramCase
from thechange-case
package, which we've been using as an all-around replacement.This is a straightforward change since it works with predictable values (values that are already declared as localized sentence strings above in the same file).
Testing Instructions