Reuse cleanEmptyObject utility and fix empty string case #49750
Conversation
youknowriad
added
the
[Type] Bug
youknowriad
requested review from
adamziel,
talldan,
tyxla,
andrewserong,
aaronrobertshaw and
gziolo
|
Size Change: -171 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
I think there's some overlap with this PR from @aaronrobertshaw - LineHeightControl: Fix application of zero values in editor I like the idea of making this function work universally (without extra params), I guess it'll be important to test that there are no regressions. I don't know what automated test coverage there is, as I'm not too familiar with the code. I'm not so sure about exporting it from the block-editor package as a long term approach, as it feels inconsistent with the kind of API block-editor usually offers, but it's private so I have no issue with it as a temporary thing. |
I agree here, this is more of a lodashy thing, I guess we could have a |
|
I'm guessing the best way to test this is to go through all the global styles (or block styles) UI (typography, colors, dimensions, border, effects, filters) and try to set a value than clear it and make sure that the result is expected. |
There was a problem hiding this comment.
Thanks for working on this @youknowriad 👍
I can see the value in reusing that utility function, so thanks for abstracting it. Perhaps it could make sense to add some tests if we're doing that, although it could be done in a separate PR.
I'd suggest reverting the Boolean() -> !== undefined change, because it seems to be breaking some expectations, as confirmed by the e2e tests. This could be something we can look at in another PR.
| @@ -33,7 +33,7 @@ export const cleanEmptyObject = ( object ) => { | |||
| const cleanedNestedObjects = Object.fromEntries( | |||
| Object.entries( object ) | |||
| .map( ( [ key, value ] ) => [ key, cleanEmptyObject( value ) ] ) | |||
| .filter( ( [ , value ] ) => Boolean( value ) ) | |||
| .filter( ( [ , value ] ) => value !== undefined ) | |||
There was a problem hiding this comment.
Noting that the comment above still says "falsy". We'll need to change that to undefined if we keep this behavior.
| @@ -33,7 +33,7 @@ export const cleanEmptyObject = ( object ) => { | |||
| const cleanedNestedObjects = Object.fromEntries( | |||
| Object.entries( object ) | |||
| .map( ( [ key, value ] ) => [ key, cleanEmptyObject( value ) ] ) | |||
| .filter( ( [ , value ] ) => Boolean( value ) ) | |||
| .filter( ( [ , value ] ) => value !== undefined ) | |||
There was a problem hiding this comment.
The concerning bit is that we don't have a good way to check all instances if they really mean to filter undefined values only.
First off, undefined might be a valid value here or there, and we might not want to clean that up. But that's fine to ignore since it was handled before and it is being handled with this new version, too.
On the other hand, though, there are some expectations that the cleaning function will also clean empty strings. You can see that there are a few failing E2E tests that confirm that problem.
So, I'd suggest reverting that change, keeping the Boolean() for now, and exploring changing it to a separate PR.
Maybe the clean function can have an argument, controlling whether to filter boolean values or undefined values, although that may seem a bit too over-engineered.
In any case - exploring it separately would make the most sense, and with that change reverted, the PR still has a lot of value in removing duplicate code and introducing a shared function that is already necessary for multiple areas of the code.
There was a problem hiding this comment.
The concerning bit is that we don't have a good way to check all instances if they really mean to filter undefined values only.
This gave me pause for concern as well over on #48917 where I was needing to allow zero values which get filtered by the Boolean() call.
Maybe the clean function can have an argument, controlling whether to filter boolean values or undefined values, although that may seem a bit too over-engineered.
#48917 followed a similar approach adding an options argument and a flag to allowZeros. We might be able to get away with the simpler option to decide between boolean or undefined checks.
In any case - exploring it separately would make the most sense, and with that change reverted, the PR still has a lot of value
+1 Agreed.
There was a problem hiding this comment.
The concerning bit is that we don't have a good way to check all instances if they really mean to filter undefined values only.
I'm counting 22 places where this function is used. It's not a public API, so extenders don't need to be taken into account. It's not impossible to manually test all those instances and make sure they behave the same as in trunk 😅
I've been testing adding/removing/resetting values in all the tools panel controls and everything seems to work as expected, plus the e2e tests are passing now, so I'm guessing that the introduction of normalizeFalsyValue has solved most of the problems.
For me, that's the most important change in this PR. I'm going to dive deeper and see why the tests are failing. I think the function was misused basically to hide some issues in other components. |
Hmm, I doubt that it's misused. For example, IIRC, |
If we're setting font family, we should be setting As you can see in #48917 we're introducing an edge case to this function to actually solve that fact that |
|
Ok, so I've looked into this further and I'm really surprised that this issue is only being raised now :). Let's take a step back and recap all what's at stake here.
Ok now, that we're all set on how the merging happens, let's take a look at how these theme.json files (or objects) get updated/edited.
Bug
Generalizing:
Solutions:
cc @aaronrobertshaw @andrewserong @talldan @nosolosw @mcsf @tyxla |
|
I'm guessing we didn't see this issue yet because most of our UI components store strings and most of them actually treat "" as "unset" but for custom CSS "" is meaningful. |
|
I think there is value in adopting the same conventions as other UI libraries — ie. treating Regarding how to represent "controlled but unset value", having a standard way would be nice, but not necessary in my opinion. Each component could find its way — for example:
I have recently posted some related thoughts in #47473 (comment) |
|
Ok, so in my last commit 2c7e4c5 I went through all the styles panels we use for global styles and block supports and made sure to actually clean the value when it's a falsy value that is supposed to be cleaned (for instance an empty string in font size...). As discussed with @jasmussen in slack, it is clear that we're lacking a "clear" link in all of these UIs as sometimes we do want both operations:
In other words, "unsetting a value" and "resetting to theme value" are two different operations and depending on the style we're manipulating in the global styles, one or the other is not available at the moment. When we introduce a clear UI solution for these two options, we should be able to remove that temporary "normalizeFalsyValue" util I just introduced. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Size Change: +236 B (0%)
Total Size: 1.37 MB
| Filename | Size | Change |
|---|---|---|
build/block-editor/content-rtl.css |
4.17 kB | +66 B (+2%) |
build/block-editor/content.css |
4.17 kB | +66 B (+2%) |
build/block-editor/index.min.js |
203 kB | +103 B (0%) |
build/block-editor/style-rtl.css |
14.6 kB | +28 B (0%) |
build/block-editor/style.css |
14.6 kB | +28 B (0%) |
build/block-library/blocks/html/editor-rtl.css |
340 B | +8 B (+2%) |
build/block-library/blocks/html/editor.css |
341 B | +8 B (+2%) |
build/block-library/editor-rtl.css |
11.6 kB | -6 B (0%) |
build/block-library/editor.css |
11.6 kB | -1 B (0%) |
build/block-library/index.min.js |
204 kB | -60 B (0%) |
build/components/index.min.js |
208 kB | +26 B (0%) |
build/dom/index.min.js |
4.76 kB | +41 B (+1%) |
build/edit-site/index.min.js |
64.2 kB | -36 B (0%) |
build/edit-site/style-rtl.css |
10.1 kB | -16 B (0%) |
build/edit-site/style.css |
10.1 kB | -17 B (0%) |
build/editor/index.min.js |
45.9 kB | -2 B (0%) |
ℹ️ View Unchanged
| Filename | Size |
|---|---|
build/a11y/index.min.js |
993 B |
build/annotations/index.min.js |
2.78 kB |
build/api-fetch/index.min.js |
2.27 kB |
build/autop/index.min.js |
2.15 kB |
build/blob/index.min.js |
483 B |
build/block-directory/index.min.js |
7.2 kB |
build/block-directory/style-rtl.css |
1.04 kB |
build/block-directory/style.css |
1.04 kB |
build/block-editor/default-editor-styles-rtl.css |
403 B |
build/block-editor/default-editor-styles.css |
403 B |
build/block-library/blocks/archives/editor-rtl.css |
61 B |
build/block-library/blocks/archives/editor.css |
60 B |
build/block-library/blocks/archives/style-rtl.css |
90 B |
build/block-library/blocks/archives/style.css |
90 B |
build/block-library/blocks/audio/editor-rtl.css |
150 B |
build/block-library/blocks/audio/editor.css |
150 B |
build/block-library/blocks/audio/style-rtl.css |
122 B |
build/block-library/blocks/audio/style.css |
122 B |
build/block-library/blocks/audio/theme-rtl.css |
138 B |
build/block-library/blocks/audio/theme.css |
138 B |
build/block-library/blocks/avatar/editor-rtl.css |
116 B |
build/block-library/blocks/avatar/editor.css |
116 B |
build/block-library/blocks/avatar/style-rtl.css |
91 B |
build/block-library/blocks/avatar/style.css |
91 B |
build/block-library/blocks/block/editor-rtl.css |
305 B |
build/block-library/blocks/block/editor.css |
305 B |
build/block-library/blocks/button/editor-rtl.css |
587 B |
build/block-library/blocks/button/editor.css |
587 B |
build/block-library/blocks/button/style-rtl.css |
628 B |
build/block-library/blocks/button/style.css |
627 B |
build/block-library/blocks/buttons/editor-rtl.css |
337 B |
build/block-library/blocks/buttons/editor.css |
337 B |
build/block-library/blocks/buttons/style-rtl.css |
332 B |
build/block-library/blocks/buttons/style.css |
332 B |
build/block-library/blocks/calendar/style-rtl.css |
239 B |
build/block-library/blocks/calendar/style.css |
239 B |
build/block-library/blocks/categories/editor-rtl.css |
113 B |
build/block-library/blocks/categories/editor.css |
112 B |
build/block-library/blocks/categories/style-rtl.css |
124 B |
build/block-library/blocks/categories/style.css |
124 B |
build/block-library/blocks/code/editor-rtl.css |
53 B |
build/block-library/blocks/code/editor.css |
53 B |
build/block-library/blocks/code/style-rtl.css |
121 B |
build/block-library/blocks/code/style.css |
121 B |
build/block-library/blocks/code/theme-rtl.css |
124 B |
build/block-library/blocks/code/theme.css |
124 B |
build/block-library/blocks/columns/editor-rtl.css |
108 B |
build/block-library/blocks/columns/editor.css |
108 B |
build/block-library/blocks/columns/style-rtl.css |
409 B |
build/block-library/blocks/columns/style.css |
409 B |
build/block-library/blocks/comment-author-avatar/editor-rtl.css |
125 B |
build/block-library/blocks/comment-author-avatar/editor.css |
125 B |
build/block-library/blocks/comment-content/style-rtl.css |
92 B |
build/block-library/blocks/comment-content/style.css |
92 B |
build/block-library/blocks/comment-template/style-rtl.css |
199 B |
build/block-library/blocks/comment-template/style.css |
198 B |
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css |
123 B |
build/block-library/blocks/comments-pagination-numbers/editor.css |
121 B |
build/block-library/blocks/comments-pagination/editor-rtl.css |
222 B |
build/block-library/blocks/comments-pagination/editor.css |
209 B |
build/block-library/blocks/comments-pagination/style-rtl.css |
235 B |
build/block-library/blocks/comments-pagination/style.css |
231 B |
build/block-library/blocks/comments-title/editor-rtl.css |
75 B |
build/block-library/blocks/comments-title/editor.css |
75 B |
build/block-library/blocks/comments/editor-rtl.css |
840 B |
build/block-library/blocks/comments/editor.css |
839 B |
build/block-library/blocks/comments/style-rtl.css |
637 B |
build/block-library/blocks/comments/style.css |
636 B |
build/block-library/blocks/cover/editor-rtl.css |
649 B |
build/block-library/blocks/cover/editor.css |
651 B |
build/block-library/blocks/cover/style-rtl.css |
1.61 kB |
build/block-library/blocks/cover/style.css |
1.6 kB |
build/block-library/blocks/details-summary/editor-rtl.css |
65 B |
build/block-library/blocks/details-summary/editor.css |
65 B |
build/block-library/blocks/details-summary/style-rtl.css |
61 B |
build/block-library/blocks/details-summary/style.css |
61 B |
build/block-library/blocks/details/style-rtl.css |
54 B |
build/block-library/blocks/details/style.css |
54 B |
build/block-library/blocks/embed/editor-rtl.css |
293 B |
build/block-library/blocks/embed/editor.css |
293 B |
build/block-library/blocks/embed/style-rtl.css |
410 B |
build/block-library/blocks/embed/style.css |
410 B |
build/block-library/blocks/embed/theme-rtl.css |
138 B |
build/block-library/blocks/embed/theme.css |
138 B |
build/block-library/blocks/file/editor-rtl.css |
300 B |
build/block-library/blocks/file/editor.css |
300 B |
build/block-library/blocks/file/style-rtl.css |
269 B |
build/block-library/blocks/file/style.css |
270 B |
build/block-library/blocks/file/view.min.js |
353 B |
build/block-library/blocks/freeform/editor-rtl.css |
2.44 kB |
build/block-library/blocks/freeform/editor.css |
2.44 kB |
build/block-library/blocks/gallery/editor-rtl.css |
984 B |
build/block-library/blocks/gallery/editor.css |
988 B |
build/block-library/blocks/gallery/style-rtl.css |
1.55 kB |
build/block-library/blocks/gallery/style.css |
1.55 kB |
build/block-library/blocks/gallery/theme-rtl.css |
122 B |
build/block-library/blocks/gallery/theme.css |
122 B |
build/block-library/blocks/group/editor-rtl.css |
654 B |
build/block-library/blocks/group/editor.css |
654 B |
build/block-library/blocks/group/style-rtl.css |
57 B |
build/block-library/blocks/group/style.css |
57 B |
build/block-library/blocks/group/theme-rtl.css |
78 B |
build/block-library/blocks/group/theme.css |
78 B |
build/block-library/blocks/heading/style-rtl.css |
76 B |
build/block-library/blocks/heading/style.css |
76 B |
build/block-library/blocks/image/editor-rtl.css |
830 B |
build/block-library/blocks/image/editor.css |
829 B |
build/block-library/blocks/image/style-rtl.css |
652 B |
build/block-library/blocks/image/style.css |
652 B |
build/block-library/blocks/image/theme-rtl.css |
137 B |
build/block-library/blocks/image/theme.css |
137 B |
build/block-library/blocks/latest-comments/style-rtl.css |
357 B |
build/block-library/blocks/latest-comments/style.css |
357 B |
build/block-library/blocks/latest-posts/editor-rtl.css |
213 B |
build/block-library/blocks/latest-posts/editor.css |
212 B |
build/block-library/blocks/latest-posts/style-rtl.css |
478 B |
build/block-library/blocks/latest-posts/style.css |
478 B |
build/block-library/blocks/list/style-rtl.css |
88 B |
build/block-library/blocks/list/style.css |
88 B |
build/block-library/blocks/media-text/editor-rtl.css |
266 B |
build/block-library/blocks/media-text/editor.css |
263 B |
build/block-library/blocks/media-text/style-rtl.css |
507 B |
build/block-library/blocks/media-text/style.css |
505 B |
build/block-library/blocks/more/editor-rtl.css |
431 B |
build/block-library/blocks/more/editor.css |
431 B |
build/block-library/blocks/navigation-link/editor-rtl.css |
716 B |
build/block-library/blocks/navigation-link/editor.css |
715 B |
build/block-library/blocks/navigation-link/style-rtl.css |
115 B |
build/block-library/blocks/navigation-link/style.css |
115 B |
build/block-library/blocks/navigation-submenu/editor-rtl.css |
299 B |
build/block-library/blocks/navigation-submenu/editor.css |
299 B |
build/block-library/blocks/navigation/editor-rtl.css |
2.13 kB |
build/block-library/blocks/navigation/editor.css |
2.14 kB |
build/block-library/blocks/navigation/style-rtl.css |
2.22 kB |
build/block-library/blocks/navigation/style.css |
2.21 kB |
build/block-library/blocks/navigation/view-modal.min.js |
2.81 kB |
build/block-library/blocks/navigation/view.min.js |
447 B |
build/block-library/blocks/nextpage/editor-rtl.css |
395 B |
build/block-library/blocks/nextpage/editor.css |
395 B |
build/block-library/blocks/page-list/editor-rtl.css |
401 B |
build/block-library/blocks/page-list/editor.css |
401 B |
build/block-library/blocks/page-list/style-rtl.css |
175 B |
build/block-library/blocks/page-list/style.css |
175 B |
build/block-library/blocks/paragraph/editor-rtl.css |
174 B |
build/block-library/blocks/paragraph/editor.css |
174 B |
build/block-library/blocks/paragraph/style-rtl.css |
279 B |
build/block-library/blocks/paragraph/style.css |
281 B |
build/block-library/blocks/post-author/style-rtl.css |
175 B |
build/block-library/blocks/post-author/style.css |
176 B |
build/block-library/blocks/post-comments-form/editor-rtl.css |
96 B |
build/block-library/blocks/post-comments-form/editor.css |
96 B |
build/block-library/blocks/post-comments-form/style-rtl.css |
501 B |
build/block-library/blocks/post-comments-form/style.css |
501 B |
build/block-library/blocks/post-date/style-rtl.css |
61 B |
build/block-library/blocks/post-date/style.css |
61 B |
build/block-library/blocks/post-excerpt/editor-rtl.css |
71 B |
build/block-library/blocks/post-excerpt/editor.css |
71 B |
build/block-library/blocks/post-excerpt/style-rtl.css |
141 B |
build/block-library/blocks/post-excerpt/style.css |
141 B |
build/block-library/blocks/post-featured-image/editor-rtl.css |
588 B |
build/block-library/blocks/post-featured-image/editor.css |
586 B |
build/block-library/blocks/post-featured-image/style-rtl.css |
322 B |
build/block-library/blocks/post-featured-image/style.css |
322 B |
build/block-library/blocks/post-navigation-link/style-rtl.css |
153 B |
build/block-library/blocks/post-navigation-link/style.css |
153 B |
build/block-library/blocks/post-template/editor-rtl.css |
99 B |
build/block-library/blocks/post-template/editor.css |
98 B |
build/block-library/blocks/post-template/style-rtl.css |
281 B |
build/block-library/blocks/post-template/style.css |
281 B |
build/block-library/blocks/post-terms/style-rtl.css |
96 B |
build/block-library/blocks/post-terms/style.css |
96 B |
build/block-library/blocks/post-time-to-read/style-rtl.css |
69 B |
build/block-library/blocks/post-time-to-read/style.css |
69 B |
build/block-library/blocks/post-title/style-rtl.css |
100 B |
build/block-library/blocks/post-title/style.css |
100 B |
build/block-library/blocks/preformatted/style-rtl.css |
103 B |
build/block-library/blocks/preformatted/style.css |
103 B |
build/block-library/blocks/pullquote/editor-rtl.css |
135 B |
build/block-library/blocks/pullquote/editor.css |
135 B |
build/block-library/blocks/pullquote/style-rtl.css |
335 B |
build/block-library/blocks/pullquote/style.css |
335 B |
build/block-library/blocks/pullquote/theme-rtl.css |
167 B |
build/block-library/blocks/pullquote/theme.css |
167 B |
build/block-library/blocks/query-pagination-numbers/editor-rtl.css |
122 B |
build/block-library/blocks/query-pagination-numbers/editor.css |
121 B |
build/block-library/blocks/query-pagination/editor-rtl.css |
221 B |
build/block-library/blocks/query-pagination/editor.css |
211 B |
build/block-library/blocks/query-pagination/style-rtl.css |
288 B |
build/block-library/blocks/query-pagination/style.css |
284 B |
build/block-library/blocks/query-title/style-rtl.css |
63 B |
build/block-library/blocks/query-title/style.css |
63 B |
build/block-library/blocks/query/editor-rtl.css |
463 B |
build/block-library/blocks/query/editor.css |
463 B |
build/block-library/blocks/quote/style-rtl.css |
222 B |
build/block-library/blocks/quote/style.css |
222 B |
build/block-library/blocks/quote/theme-rtl.css |
223 B |
build/block-library/blocks/quote/theme.css |
226 B |
build/block-library/blocks/read-more/style-rtl.css |
132 B |
build/block-library/blocks/read-more/style.css |
132 B |
build/block-library/blocks/rss/editor-rtl.css |
149 B |
build/block-library/blocks/rss/editor.css |
149 B |
build/block-library/blocks/rss/style-rtl.css |
289 B |
build/block-library/blocks/rss/style.css |
288 B |
build/block-library/blocks/search/editor-rtl.css |
165 B |
build/block-library/blocks/search/editor.css |
165 B |
build/block-library/blocks/search/style-rtl.css |
408 B |
build/block-library/blocks/search/style.css |
406 B |
build/block-library/blocks/search/theme-rtl.css |
114 B |
build/block-library/blocks/search/theme.css |
114 B |
build/block-library/blocks/separator/editor-rtl.css |
146 B |
build/block-library/blocks/separator/editor.css |
146 B |
build/block-library/blocks/separator/style-rtl.css |
234 B |
build/block-library/blocks/separator/style.css |
234 B |
build/block-library/blocks/separator/theme-rtl.css |
194 B |
build/block-library/blocks/separator/theme.css |
194 B |
build/block-library/blocks/shortcode/editor-rtl.css |
329 B |
build/block-library/blocks/shortcode/editor.css |
329 B |
build/block-library/blocks/site-logo/editor-rtl.css |
489 B |
build/block-library/blocks/site-logo/editor.css |
489 B |
build/block-library/blocks/site-logo/style-rtl.css |
203 B |
build/block-library/blocks/site-logo/style.css |
203 B |
build/block-library/blocks/site-tagline/editor-rtl.css |
86 B |
build/block-library/blocks/site-tagline/editor.css |
86 B |
build/block-library/blocks/site-title/editor-rtl.css |
116 B |
build/block-library/blocks/site-title/editor.css |
116 B |
build/block-library/blocks/site-title/style-rtl.css |
57 B |
build/block-library/blocks/site-title/style.css |
57 B |
build/block-library/blocks/social-link/editor-rtl.css |
184 B |
build/block-library/blocks/social-link/editor.css |
184 B |
build/block-library/blocks/social-links/editor-rtl.css |
674 B |
build/block-library/blocks/social-links/editor.css |
673 B |
build/block-library/blocks/social-links/style-rtl.css |
1.4 kB |
build/block-library/blocks/social-links/style.css |
1.39 kB |
build/block-library/blocks/spacer/editor-rtl.css |
359 B |
build/block-library/blocks/spacer/editor.css |
359 B |
build/block-library/blocks/spacer/style-rtl.css |
48 B |
build/block-library/blocks/spacer/style.css |
48 B |
build/block-library/blocks/table/editor-rtl.css |
433 B |
build/block-library/blocks/table/editor.css |
433 B |
build/block-library/blocks/table/style-rtl.css |
651 B |
build/block-library/blocks/table/style.css |
650 B |
build/block-library/blocks/table/theme-rtl.css |
157 B |
build/block-library/blocks/table/theme.css |
157 B |
build/block-library/blocks/tag-cloud/style-rtl.css |
251 B |
build/block-library/blocks/tag-cloud/style.css |
253 B |
build/block-library/blocks/template-part/editor-rtl.css |
404 B |
build/block-library/blocks/template-part/editor.css |
404 B |
build/block-library/blocks/template-part/theme-rtl.css |
101 B |
build/block-library/blocks/template-part/theme.css |
101 B |
build/block-library/blocks/text-columns/editor-rtl.css |
95 B |
build/block-library/blocks/text-columns/editor.css |
95 B |
build/block-library/blocks/text-columns/style-rtl.css |
166 B |
build/block-library/blocks/text-columns/style.css |
166 B |
build/block-library/blocks/verse/style-rtl.css |
99 B |
build/block-library/blocks/verse/style.css |
99 B |
build/block-library/blocks/video/editor-rtl.css |
552 B |
build/block-library/blocks/video/editor.css |
555 B |
build/block-library/blocks/video/style-rtl.css |
179 B |
build/block-library/blocks/video/style.css |
179 B |
build/block-library/blocks/video/theme-rtl.css |
139 B |
build/block-library/blocks/video/theme.css |
139 B |
build/block-library/classic-rtl.css |
179 B |
build/block-library/classic.css |
179 B |
build/block-library/common-rtl.css |
1.12 kB |
build/block-library/common.css |
1.12 kB |
build/block-library/editor-elements-rtl.css |
75 B |
build/block-library/editor-elements.css |
75 B |
build/block-library/elements-rtl.css |
54 B |
build/block-library/elements.css |
54 B |
build/block-library/reset-rtl.css |
478 B |
build/block-library/reset.css |
478 B |
build/block-library/style-rtl.css |
12.8 kB |
build/block-library/style.css |
12.8 kB |
build/block-library/theme-rtl.css |
698 B |
build/block-library/theme.css |
703 B |
build/block-serialization-default-parser/index.min.js |
1.13 kB |
build/block-serialization-spec-parser/index.min.js |
2.83 kB |
build/blocks/index.min.js |
51.1 kB |
build/commands/index.min.js |
14.8 kB |
build/commands/style-rtl.css |
1.1 kB |
build/commands/style.css |
1.09 kB |
build/components/style-rtl.css |
11.7 kB |
build/components/style.css |
11.7 kB |
build/compose/index.min.js |
12.4 kB |
build/core-data/index.min.js |
16.3 kB |
build/customize-widgets/index.min.js |
12.2 kB |
build/customize-widgets/style-rtl.css |
1.41 kB |
build/customize-widgets/style.css |
1.41 kB |
build/data-controls/index.min.js |
718 B |
build/data/index.min.js |
8.68 kB |
build/date/index.min.js |
40.4 kB |
build/deprecated/index.min.js |
518 B |
build/dom-ready/index.min.js |
336 B |
build/edit-post/classic-rtl.css |
571 B |
build/edit-post/classic.css |
571 B |
build/edit-post/index.min.js |
35 kB |
build/edit-post/style-rtl.css |
7.59 kB |
build/edit-post/style.css |
7.58 kB |
build/edit-widgets/index.min.js |
17.3 kB |
build/edit-widgets/style-rtl.css |
4.56 kB |
build/edit-widgets/style.css |
4.56 kB |
build/editor/style-rtl.css |
3.49 kB |
build/editor/style.css |
3.48 kB |
build/element/index.min.js |
4.95 kB |
build/escape-html/index.min.js |
548 B |
build/format-library/index.min.js |
7.26 kB |
build/format-library/style-rtl.css |
557 B |
build/format-library/style.css |
556 B |
build/hooks/index.min.js |
1.66 kB |
build/html-entities/index.min.js |
454 B |
build/i18n/index.min.js |
3.79 kB |
build/is-shallow-equal/index.min.js |
535 B |
build/keyboard-shortcuts/index.min.js |
1.79 kB |
build/keycodes/index.min.js |
1.94 kB |
build/list-reusable-blocks/index.min.js |
2.14 kB |
build/list-reusable-blocks/style-rtl.css |
865 B |
build/list-reusable-blocks/style.css |
865 B |
build/media-utils/index.min.js |
2.99 kB |
build/notices/index.min.js |
977 B |
build/plugins/index.min.js |
1.94 kB |
build/preferences-persistence/index.min.js |
2.23 kB |
build/preferences/index.min.js |
1.35 kB |
build/primitives/index.min.js |
960 B |
build/priority-queue/index.min.js |
1.52 kB |
build/private-apis/index.min.js |
942 B |
build/react-i18n/index.min.js |
702 B |
build/react-refresh-entry/index.min.js |
8.44 kB |
build/react-refresh-runtime/index.min.js |
7.31 kB |
build/redux-routine/index.min.js |
2.75 kB |
build/reusable-blocks/index.min.js |
2.26 kB |
build/reusable-blocks/style-rtl.css |
265 B |
build/reusable-blocks/style.css |
265 B |
build/rich-text/index.min.js |
11.1 kB |
build/server-side-render/index.min.js |
2.09 kB |
build/shortcode/index.min.js |
1.52 kB |
build/style-engine/index.min.js |
1.55 kB |
build/token-list/index.min.js |
650 B |
build/url/index.min.js |
3.74 kB |
build/vendors/inert-polyfill.min.js |
2.48 kB |
build/vendors/react-dom.min.js |
41.8 kB |
build/vendors/react.min.js |
4.02 kB |
build/viewport/index.min.js |
1.09 kB |
build/warning/index.min.js |
280 B |
build/widgets/index.min.js |
7.3 kB |
build/widgets/style-rtl.css |
1.18 kB |
build/widgets/style.css |
1.18 kB |
build/wordcount/index.min.js |
1.06 kB |
|
It is also interesting to note that to solve this particular issue, several styles had to introduce an adhoc solution:
It seems we need a generic solution both in the UI and in the theme.json to solve these cases in a common way. |
Just to follow up on this one, and to make sure I'm understanding it right, what I would expect is that if I go into Global Styles and explicitly selects a value in an input field, clear it, and save, that value is effectively unset. If I want to get the theme.json default back, I would expect to use the ellipsis menu > Reset to default option. This one:
I recognize the limitation we have, this is partially an issue whe ToolsPanel ellipsis menu and something @jameskoster has often brought up, we are lacking an explicit "unset" action for many of these tools. While I can clear an input field, I cannot clear a slider to its unset state. We have an adjacent task of designing a way to show inheritance as well, between these topics there might be an opportunity to consolidate. |
That is my expectation too but it's not how it work today for most fields. (You can try even in trunk, this PR doesn't change that at the moment) |
cc/ @glendaviesnz the author of the CustomCSS feature for awareness. |
|
I think this PR is ready for a final review. All issues are addressed I think. |
youknowriad
force-pushed
the
update/clean-empty-object
branch
from
2c7e4c5
to
b4ed784
Compare
|
Flaky tests detected in b4ed784c21ae7cc47fa0a70a93b1d396f9a9b632. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4743120341
|
I think this is the right approach too. Style configurations are a stable thing that should retain simple mechanics so as to be predictable for everyone (its main job it to be good at merging and work across the server/client boundary), so it should be left up to external setters to clean up the values. |
| immutableSet( | ||
| value, | ||
| [ 'layout', 'contentSize' ], | ||
| normalizeFalsyValue( newValue ) |
There was a problem hiding this comment.
I'm undecided between this and something explicit and immediate like newValue || undefined. While I understand the value of having a function like normalizeFalsyValue acting as an "anchor" (complete with documentation and the rationale for normalisation), I'm not sure that function name is descriptive enough.
That said, you could say I am picking nits; I'm wondering about how to make all the hidden traps of Global Styles as evident as possible.
There was a problem hiding this comment.
I agree that it's not a perfect solution, my main motivation is the documentation actually but I'm also undecided, especially since IMO we should be moving away slowly from its usage.
There was a problem hiding this comment.
I think the undefined solution makes a lot of sense 👍
However, I personally think that:
- There are far too many
|| undefinedto generalize this as a usable abstraction, and while here we're starting to utilize ``normalizeFalsyValue` in a few places, there are still way more that will remain as they are. || undefinedis far too straightforward and simple to be generalized, and I think is more readable as-is instead of going throughnormalizeFalsyValue()
That being said, I'd personally lean towards inlining || undefined everywhere instead of introducing a function to do it. I think having it inline is more readable and comprehensible than abstracting it in a function.
There was a problem hiding this comment.
Sorry I merged and missed this comment. I can remove that function soon if you feel strongly about it. I'm not sure how to handle the loss of documentation thought without a centralized function.
There was a problem hiding this comment.
If you don't mind, I'd prefer removing it, yes. I'm not sure there's a good need for documentation on this part, really. || undefined seems quite straightforward as a JS idiom and even if we abstract it to a function with docs (like we did), this won't help ensure that folks are using it where necessary. I'm also again pointing to the tens of usages of || undefined in the codebase where the usage isn't documented but is still there intentionally. Maybe I just fail to understand what you consider miscommunicated with the lack of documentation in these instances?
There was a problem hiding this comment.
What I wanted to communicate is that for styles UI (most of them), clearing inputs means reset to the theme.json values. So it's not just a random JS operation to replace empty strings with undefined, it has bigger impacts.
There was a problem hiding this comment.
Opened this #50033
There was a problem hiding this comment.
Is this something we should document at a different place then - for example in the primary README of the global styles component in the block editor? I'm just not sure if documenting it within a separate helper function will help much with adopting it, especially when folks aren't aware that they should be using that function.
There was a problem hiding this comment.
This is looking pretty close to me! My manual testing of the tools panels, Cover block transforms and deprecated Query block migration didn't bring up any issues.
One minor detail is the sole unit test for this function (that I could find) lives in the test file for the color hook. Would be better to move it into the utils file. Might also be good to add a couple more tests now that the function isn't expected to strip things like false and empty string 🙂
| * Ideally, falsy values should be retained but the current implementation of | ||
| * global styles merging (theme with user) relies on the fact that emptying inputs | ||
| * (empty strings) should fallback to the parent value. | ||
| * Ideally, there should be a dedicated UI element to "revert to theme" for each input instead. |
There was a problem hiding this comment.
I'm probably missing something, but isn't that what the "reset" buttons in the tools panel dropdowns are meant to do?
There was a problem hiding this comment.
The reset functionality of the ToolsPanel menu is intended to facilitate setting any given value back to undefined allowing it to fall back to the global style or theme value. The reset callback triggered by the panel's menu items, could really set it to any value though.
relies on the fact that emptying inputs (empty strings) should fallback to the parent value
I think this part of the comment is referring to when a control can be manually cleared by a user e.g. deleting the value within an input control. In this case, it is retained by the new cleanEmptyObject but for the global styles to fall back to the theme's value correctly, we need to ensure a user value isn't set.
Either way, I could be misunderstanding. Perhaps we could add some further clarity to this comment?
There was a problem hiding this comment.
Yes, I meant that each "style" should have its own dedicated "reset" link, similar to the global one. and I agree that the comment can be made clearer.
| @@ -33,7 +33,7 @@ export const cleanEmptyObject = ( object ) => { | |||
| const cleanedNestedObjects = Object.fromEntries( | |||
| Object.entries( object ) | |||
| .map( ( [ key, value ] ) => [ key, cleanEmptyObject( value ) ] ) | |||
| .filter( ( [ , value ] ) => Boolean( value ) ) | |||
| .filter( ( [ , value ] ) => value !== undefined ) | |||
There was a problem hiding this comment.
The concerning bit is that we don't have a good way to check all instances if they really mean to filter undefined values only.
I'm counting 22 places where this function is used. It's not a public API, so extenders don't need to be taken into account. It's not impossible to manually test all those instances and make sure they behave the same as in trunk 😅
I've been testing adding/removing/resetting values in all the tools panel controls and everything seems to work as expected, plus the e2e tests are passing now, so I'm guessing that the introduction of normalizeFalsyValue has solved most of the problems.
There was a problem hiding this comment.
I've gone through and tested anywhere that I found cleanEmptyObject being used. It's testing well for me and I didn't spot any differences between this PR and trunk.
I don't have any strong opinions regarding the use of normalizeFalsyValue or something explicit like newValue || undefined, I'll defer to others on the best approach there.
The uses of cleanEmptyObject I tested include:
- Generic block supports panel added via our InspectorControls group slots
- Dimensions Panel - Global Styles
- Dimensions Panel - Block editor
- Border Panel
- Typography Panel
- Position Panel
- FontSizeEdit
- LineHeightEdit
- Cover transforms
- Query block deprecation
- Migrate font family util
- useGlobalStylesUserConfig
The above list isn't 22 places as noted here so perhaps I'm missing something. More eyes and testing might be beneficial.
Nice work tidying this up @youknowriad, thank you 👍
| * Ideally, falsy values should be retained but the current implementation of | ||
| * global styles merging (theme with user) relies on the fact that emptying inputs | ||
| * (empty strings) should fallback to the parent value. | ||
| * Ideally, there should be a dedicated UI element to "revert to theme" for each input instead. |
There was a problem hiding this comment.
The reset functionality of the ToolsPanel menu is intended to facilitate setting any given value back to undefined allowing it to fall back to the global style or theme value. The reset callback triggered by the panel's menu items, could really set it to any value though.
relies on the fact that emptying inputs (empty strings) should fallback to the parent value
I think this part of the comment is referring to when a control can be manually cleared by a user e.g. deleting the value within an input control. In this case, it is retained by the new cleanEmptyObject but for the global styles to fall back to the theme's value correctly, we need to ensure a user value isn't set.
Either way, I could be misunderstanding. Perhaps we could add some further clarity to this comment?
youknowriad
force-pushed
the
update/clean-empty-object
branch
from
bd93d11
to
f154e5c
Compare
|
I think the fact the function had a unit test and that the test only checked for "undefined" values previously is a good indication that this function was never meant to discard falsy values :) |
This change doesn't appear to affect the existing Global CSS functionality at all - was able to unset the theme CSS still, and revert with the |
|
@youknowriad I see this was just merged, but just wanted to pick your thoughts on #49750 (comment) and whether we can simplify the code by removing the |
WHAT
🤖 Generated by Copilot at c633931
This pull request refactors the codebase to use a single
cleanEmptyObjectfunction from theblock-editorpackage, instead of multiple copies or local versions. This function removes empty objects from block attributes and global styles, and was improved to handle undefined values better. The pull request also deletes unused code and updates imports accordingly.🤖 Generated by Copilot at c633931
WHY
In this PR, I noticed that we treat "empty strings" and "undefined" values similarly in this util, which causes bugs sometimes where we drop values that are meaningful. IMO an empty string is meaningful, a "false" value is also meaningful (maybe the default is true for some block attribute). So far, that problem was not that visible I guess because we mainly use this utility to "clean" the "style" attribute, but it turns out that we also use it for global styles and omitting empty strings causes the fallback (theme.json global styles) to trigger as seen on the comment above. I think a better behavior is to only clean "undefined" values.
There's a small risk in this PR in creating some lingering block attributes in the comment demarcation that might not have been there before. If it happens we should ensure that the UI controls actually set "undefined" when "clearing" values instead of setting empty strings. Some testing is needed to solve these potential issues.
The other thing this PR does is to actually ship this util as a private API of the block editor package as I saw that it was duplicated three times in our code base.
HOW
🤖 Generated by Copilot at c633931
cleanEmptyObjectfunction from various modules toblock-editorpackage and use stricter check for undefined values (link, link, link, link, link, link, link, link, link, link, link)isEmptyimport fromglobal-styles-providermodule (link)