-
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
Try absorbing padding for cover #23032
Closed
Closed
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev
Previous commit
Remove padding preset values
- Loading branch information
commit 744a5f3eb6f622bb23dc4ed8528e6388e33309e3
There are no files selected for viewing
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
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.
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.
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:<>
should be resolved to the variable and that we expose them via avariables
subkey of the global scope (I'm not attached to this specific format). Theme authors could then do: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.
Is the intent to ensure compat for browsers with no
var()
support? Or is it something else?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.
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.
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.
Yes, it's not an easy decision.
Can this be mitigated by properly namespacing them, even if they are globally available?
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.
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).
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.
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.
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'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?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.
@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
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.
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:
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.
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(likecalc()
) is the main blocker for native mobile. Convo going on here: #24165We'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. ✌️