Lodash: Remove completely from @wordpress/style-engine package
#51726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Type] Performance
tyxla
requested review from
Mamaduka,
sgomes,
aristath,
jsnajdr,
swissspidy,
glendaviesnz,
ramonjd,
flootr,
chad1008,
ntsekouras and
fabiankaegy
|
Size Change: +397 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in df988f2465530105233f8165916f4daed6f1bcc9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5340321623
|
chad1008
reviewed
Jun 21, 2023
There was a problem hiding this comment.
Changes look good to me and test well for the most part... I did find that links within paragraph blocks weren't registering custom colors for their default state. They pick up hover-state color and other changes with no problem, the default color just seemed stuck.
Other links like those generated for archives, login, comments, etc. all picked up any styles I threw at them with no problem, so it's not that the color isn't saving. Could be fluke on my end, curious if anyone else sees the same results.
|
Thanks for testing so thoroughly, @chad1008! I wonder if this is something you can reproduce on trunk too? |
|
@tyxla Thank you for this. Tests well for me. 🙇 Hope you don't mind, but I threw in a few extra tests for the kebabCase on presets.
I would be interested to know how to reproduce. I tried and I couldn't. Screencast
|
|
By the way... e2e fails are unrelated |
ramonjd
approved these changes
Jun 22, 2023
chad1008
approved these changes
Jun 22, 2023
There was a problem hiding this comment.
Re-ran some tests from yesterday and seeing the same... it works great in the actual site editor, the issue I found was:
- Create post, add paragraph with a link.
- Publish post and view on front end. Note link color.
- Launch site editor and change link styles, specifically colors.
- Refresh front end view of the test post.
In my tests, all theme-generated links look great. The in-content link seems to register every style I tested except for the main color... hover color gets picked up but the default state of the link maintains the theme/preset color.
All of that said - I can reproduce it on trunk, sorry for not thinking to check that yesterday. Whether it's a fluke on my end or something else, it definitely doesn't look like it's related to this PR, so adding my approval as well 👍
aristath
approved these changes
Jun 23, 2023
tyxla
force-pushed
the
remove/lodash-style-engine
branch
from
df988f2
to
e7c6e88
Compare
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…dPress#51726) * Style Engine: Add change-case dependency * Style Engine: Migrate kebabCase in compileCSS() * Style Engine: Migrate kebabCase in getCSSVarFromStyleValue * Style Engine: Refactor away from _.get() * Style Engine: Remove Lodash dependency * Added a few more unit tests for getCSSVarFromStyleValue() --------- Co-authored-by: ramon <ramonjd@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR refactors the
@wordpress/style-enginepackage away from Lodash completely. There are just a few usages of_.get()and_.kebabCase().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-fetchpackage haslodashas a dependency #39495How?
_.get().paramCasefromchange-caseinstead ofkebabCase, which we've already been utilizing across the repository.Testing Instructions