Skip to content
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: Remove duplicate style mappings from global styles #25145

Conversation

jorgefilipecosta
Copy link
Member

This updates the global styles compile mechanism to take advantage of the centralized style mappings instead of duplicating them.

How has this been tested?

I verified the global styles sidebar still works as expected.

@github-actions
Copy link

github-actions bot commented Sep 8, 2020

Size Change: -12 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/annotations/index.js 3.67 kB -1 B
build/block-editor/index.js 128 kB -1 B
build/block-library/index.js 138 kB +2 B (0%)
build/components/index.js 200 kB -4 B (0%)
build/data/index.js 8.54 kB -1 B
build/edit-navigation/index.js 11.7 kB -4 B (0%)
build/edit-site/index.js 19.4 kB -8 B (0%)
build/edit-widgets/index.js 12.1 kB +8 B (0%)
build/editor/index.js 45.6 kB -2 B (0%)
build/element/index.js 4.64 kB +2 B (0%)
build/i18n/index.js 3.56 kB -1 B
build/keyboard-shortcuts/index.js 2.52 kB -1 B
build/keycodes/index.js 1.94 kB -2 B (0%)
build/notices/index.js 1.79 kB +2 B (0%)
build/plugins/index.js 2.56 kB -1 B
build/rich-text/index.js 13.9 kB +1 B
build/server-side-render/index.js 2.77 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 8.5 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.64 kB 0 B
build/block-library/editor.css 8.64 kB 0 B
build/block-library/style-rtl.css 7.59 kB 0 B
build/block-library/style.css 7.58 kB 0 B
build/block-library/theme-rtl.css 754 B 0 B
build/block-library/theme.css 754 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.7 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 305 kB 0 B
build/edit-post/style-rtl.css 6.26 kB 0 B
build/edit-post/style.css 6.25 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

return reduce(
__EXPERIMENTAL_STYLE_MAPPINGS,
function ( declarations, stylePath, rawKey ) {
const key = kebabCase( rawKey );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the docs, kebabCase("--wp--style--color--link") becomes wp-style-color-link, which causes issues both with blockSupports check and generating the declaration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch the issue was fixed 👍

@@ -1,22 +1,9 @@
/* CSS properties */
export const FONT_SIZE = 'font-size';
export const LINK_COLOR = '--wp--style--color--link';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we move the link color prop as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link color is used on this file and the other file so it makes sense to keep it as is and share it.

} from '../editor/utils';
import { LINK_COLOR } from '../editor/utils';

const BACKGROUND_COLOR = 'background-color';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these variables be moved to hooks/utils as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine to use background-color when needed. We are at the panel label so probably the panel should know the supports key is background-color. Moving the variables would only mean one uses BACKGROUND_COLOR instead of background-color which is not a big win.

@oandregal
Copy link
Member

The color and typography panels access the properties they need by using the strings color.link and typography.lineHeight. I guess we also need to centralize them as well.

@oandregal
Copy link
Member

oandregal commented Sep 8, 2020

The STYLE_MAPPINGS defined in block-editor/hooks/utils are used for deserializing/serializing two different sources of information, the style attribute of blocks and theme.json data. I agree they having the same shape makes sense. However, I wonder if a better place for this information is the Block API (blocks package) instead. It seems to me that's a better place to consolidate this kind of information as well as documenting it in the handbook.

@jorgefilipecosta jorgefilipecosta force-pushed the update/remove-duplicate-style-mappings-from-global-styles branch from 9fa5fb4 to 2980b27 Compare September 8, 2020 15:44
@jorgefilipecosta
Copy link
Member Author

The STYLE_MAPPINGS defined in block-editor/hooks/utils are used for deserializing/serializing two different sources of information, the style attribute of blocks and theme.json data. I agree they having the same shape makes sense. However, I wonder if a better place for this information is the Block API (blocks package) instead. It seems to me that's a better place to consolidate this kind of information as well as documenting it in the handbook.

I think both the style attributes of the block and the theme.json are the same thing. They are styles just saved in different locations (for now). So I guess we should promote reusability but in the future I expect the component used in style attributes and the components used in global styles become the same. I guess these components will probably be part of the block editor. As I think the path will probably the block editor exposing functionality used by global styles I think for now we can try to add things in the block editor. I expect in the future we may not need to expose the mappings and just expose components/functions from block editor.

@jorgefilipecosta
Copy link
Member Author

The color and typography panels access the properties they need by using the strings color.link and typography.lineHeight. I guess we also need to centralize them as well.

Trying the follow this feedback I end up submitting PR #25159, as currently, things are not in the array format so we could not use the paths we have in the central mapping.

@oandregal
Copy link
Member

Hey @jorgefilipecosta I've slept on this and created an alternative at #25185 It builds upon some of the ideas you proposed here and added others that I missed :)

@jorgefilipecosta
Copy link
Member Author

Close in favor of #25185.

@youknowriad youknowriad deleted the update/remove-duplicate-style-mappings-from-global-styles branch September 14, 2020 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants