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

Try absorbing padding for cover #23032

Closed
wants to merge 3 commits into from
Closed

Conversation

oandregal
Copy link
Member

Follow-up on #21492

Work In Progress

This is an in-progress experiment to absorb style declarations that were previously included in the block's CSS via theme.json. By managing the block's CSS we can consolidate the style needs of different origins (block defaults, theme, and user), ship less CSS to the browser (more performant), and adapt the CSS to the different contexts (editor, front) without any theme intervention.

@oandregal oandregal self-assigned this Jun 9, 2020
@github-actions
Copy link

github-actions bot commented Jun 9, 2020

Size Change: -13 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-library/style-rtl.css 7.71 kB -6 B (0%)
build/block-library/style.css 7.72 kB -7 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.77 kB 0 B
build/block-directory/style-rtl.css 892 B 0 B
build/block-directory/style.css 892 B 0 B
build/block-editor/index.js 106 kB 0 B
build/block-editor/style-rtl.css 11.8 kB 0 B
build/block-editor/style.css 11.8 kB 0 B
build/block-library/editor-rtl.css 7.88 kB 0 B
build/block-library/editor.css 7.89 kB 0 B
build/block-library/index.js 127 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 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 48.1 kB 0 B
build/components/index.js 195 kB 0 B
build/components/style-rtl.css 19.5 kB 0 B
build/components/style.css 19.5 kB 0 B
build/compose/index.js 9.31 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.25 kB 0 B
build/edit-navigation/style-rtl.css 918 B 0 B
build/edit-navigation/style.css 919 B 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.6 kB 0 B
build/edit-post/style.css 5.6 kB 0 B
build/edit-site/index.js 15.5 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/index.js 9.34 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.3 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.41 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 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.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

"core/cover": {
"styles": {
"spacing": {
"padding": 16
Copy link
Member Author

@oandregal oandregal Jun 9, 2020

Choose a reason for hiding this comment

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

This is how we'd absorb the existing CSS properties of the blocks. A few thoughts on this:

First, I think I'd wait post-5.5 to do this kind of change, so we have a full cycle to test in the plugin, as this requires open-up theme.json to production. Related conversation: for the link color feature to work, we need to expose the presets declared by the theme via add_theme_support as CSS variables. We may be able to do this by only shipping to WordPress core the function that transforms the theme support declarations into CSS variables, without the need of shipping theme.json just yet. I thought I'd connect this two convos for perspective.

A second point to talk about is that, ideally, we can reference this sort of variables ($grid-unit-20) via theme.json. We can also leave as it is, hardcoded values, but I think it'll be easier if we can use "named variables". Two ways we can do this are:

  1. We add the grid unit values as if they were any other presets (font-size, color) in theme.json, and theme authors will reference them as they do with any other preset:
{
  "global": {
    "presets": {
      "padding": [
          {
              "slug": "unit",
              "value": 8
          },
          {
              "slug": "unit-20"
              "value": 16
          }
      ]
    }
  },
  "core/cover": {
    "styles": {
      "spacing": {
        "padding": "var(--wp--preset--padding--unit-20)"
      }
    }
  }
}
  1. We create a mechanism to resolve the variable at render time. We need to mark the values to be resolved as well as having those variables available somewhere. Let's pretend that all values that are enclosed by <> should be resolved to the variable and that we expose them via a variables subkey of the global scope (I'm not attached to this specific format). Theme authors could then do:
{
  "global": {
    "variables": {
      "grid-unit": 8,
      "grid-unit-20": 16
    }
  },
  "core/cover": {
    "styles": {
      "spacing": {
        "padding": "<global.variables.grid-unit-20>"
      }
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We create a mechanism to resolve the variable at render time.

Is the intent to ensure compat for browsers with no var() support? Or is it something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's something that crossed my mind, yes, although we're already using CSS vars in other places (presets), so we need to provide fallbacks for this sort of situation anyway.

One advantage the second approach has it that it doesn't pollute the global space with all the variables that are present in the base-styles. However, I don't know how much a problem that would be in practice. On the other hand, the second approach requires a specific syntax, which I consider a disadvantage. I like how the first approach requires fewer concepts to learn: leaf values just use regular CSS syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's not an easy decision.

it doesn't pollute the global space with all the variables that are present in the base-styles.

Can this be mitigated by properly namespacing them, even if they are globally available?

Copy link
Member Author

Choose a reason for hiding this comment

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

More brain-dumping: we'll need to expose the theme.json via some REST endpoint for mobile to be able to use/expose this data in the apps. One consideration we should make is how to balance theme authors' needs (use already-known CSS syntax at the leaf nodes) vs mobile code's needs (use parseable syntax at the leaf nodes).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @nosolosw for your consideration on mobile end. It should be possible to evaluate formulas with basic operators in it but not sure how to evaluate the css functions on mobile side, I don't think there's an easy way :( cc @dratwas @maxme @hypest if you want to bring a different point of view.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how to evaluate the css functions on mobile side

Can't say I have good insight on the matter, but I wonder, what CSS functions are we perhaps talking about? Talking about the var() specifically or others too?

Copy link
Member Author

Choose a reason for hiding this comment

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

@pinarol has been helping me levelling up a bit on styling for the mobile app. I think we can move the discussion over this issue #24165 which I also listed in the tracking issue for the work related to the block style system #20331

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the usage of CSS syntax "var(--wp--preset--padding--unit-20)". We would need to parse things anyway no matter if it is "var(--wp--preset--padding--unit-20)" or "<global.variables.grid-unit-20>". And in that case, I would prefer if we used what developers already know instead of needing to specify a new syntax and a parser for it.

So I guess things could look like this:

{
  "global": {
    "variables": {
      "grid-unit": 8,
      "grid-unit-20": 16
    }
  },
  "core/cover": {
    "styles": {
      "spacing": {
        "padding": "var(--wp--global--variables--grid-unit-20)"
      }
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't worry if it was just about var() actually, as you said we need to parse things anyway. But the possibility of using any kind of css function(like calc()) is the main blocker for native mobile. Convo going on here: #24165

We'll be starting to actively work on GSS for native mobile in a couple of weeks so hopefully we'll have more progress on that issue. ✌️

@oandregal oandregal added the [Status] In Progress Tracking issues with work in progress label Jun 12, 2020
Base automatically changed from master to trunk March 1, 2021 15:43
@oandregal
Copy link
Member Author

Going to close this for now. We can always re-open. While I like the idea of absorbing more of the block's CSS into the GS engine, this doesn't seem the direction to go. In a conversation with @youknowriad he argued that the styles of the block should live with the block. I agree with that. I'm not sure what form should it take this "absorbing the block's css into the GS engine" idea: can we set block attributes via theme.json? do we generate a dynamic rule for the block depending on theme.json values (not part of the block's style.css)? Perhaps we can revisit once theme.json lands into core.

@oandregal oandregal closed this Mar 10, 2021
@oandregal oandregal deleted the try/absorb-padding-cover branch March 10, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants