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

WP_Theme_JSON classes: clarify status of position.fixed #48635

Closed
oandregal opened this issue Mar 1, 2023 · 4 comments · Fixed by #48660
Closed

WP_Theme_JSON classes: clarify status of position.fixed #48635

oandregal opened this issue Mar 1, 2023 · 4 comments · Fixed by #48660
Assignees
Labels
[Type] Question Questions about the design or development of the editor.

Comments

@oandregal
Copy link
Member

What problem does this address?

In comparing Gutenberg and WordPress core versions of the WP_Theme_JSON class, I realized the position.sticked setting/style was backported but the postition.fixed is in a mixed position.

What I know

This core ticket https://core.trac.wordpress.org/ticket/57618 and corresponding PR WordPress/wordpress-develop#3973 backported to core support for position.sticky, where it's mentioned that:

The backport deviates from Gutenberg trunk in one way: fixed position type is excluded from the appearance tools opt-in: this is because fixed positioning isn't being opted-in for the Group block and isn't ready to be used as a default opt-in. The code paths exist for fixed positioning as it will be supported in the future, but for now, sticky is the only one exposed by default. (The overall block support should still support it as a valid value, though)

However, WordPress core still considers position.fixed a valid setting, as per https://github.com/WordPress/wordpress-develop/pull/3973/files#diff-e12c3008d747d1bec5be9927c5e7b1ced0a88641abe52c278d495da936714817R343

Next steps

I don't know.

Without having the full context, I presume we may need to:

  • WordPress core: do not have postition.fixed as a valid setting?

  • Gutenberg: if making Gutenberg work like core is not feasible, at least we need to document the situation. In other cases where we don't want to backport some code we add a comment, like we do for fonts and appearance tools.

@oandregal
Copy link
Member Author

cc people who were involved in this work @andrewserong @tellthemachines @jameskoster @aaronrobertshaw @carolinan

Sorry if this is already clear for everyone else. Certainly, it is not for me, so I thought I'd share to avoid bugs due to under/over-backporting in the future.

@oandregal oandregal added the [Type] Question Questions about the design or development of the editor. label Mar 1, 2023
@andrewserong
Copy link
Contributor

Thanks for the ping @oandregal!

at least we need to document the situation

Good idea to add some documentation. I'll try to recap the current state:

  • In Gutenberg, appearance tools automatically opts in to both fixed and sticky position settings, as part of work that will extend beyond 6.2 (for 6.3 it'd be great to properly support fixed in core blocks)
  • For 6.2, fixed and sticky are both valid settings, so for example, if a custom block really wanted to use fixed and provided their own CSS to make it work properly (e.g. adding width values), they potentially could. It'd be up to the custom block to add some additional rules to make it work, and it isn't advertised per-se, but the value is allowed, and will be output accordingly. Any themes wanting to use it would also need to explicitly opt-in — it's a way to make the feature technically available while ensuring that no-one accidentally starts using it.
  • In Gutenberg, we want to be a little more permissive so that it's easier to start using it once we experiment with it further in Gutenberg via core blocks like Group.

Another way of putting it would be: imagine if what's in core is what's currently in Gutenberg, and then I open up a PR to propose that for 6.3 work it'd be easier if appearanceTools opted-in to fixed as well. I think that's the current state in Gutenberg. So for a next step, I think we probably just need to add the // BEGIN EXPERIMENTAL CODE comment on the line for position.fixed in appearance tools.

What do you think? I can open up a quick PR to do that.

@andrewserong
Copy link
Contributor

I've opened up a PR to clarify the status of the opt-in over in: #48660

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Mar 1, 2023
@oandregal
Copy link
Member Author

Thanks for the context, Andrew, that clarifies it a bit more for me.

In Gutenberg, appearance tools automatically opts in to both fixed and sticky position settings, as part of work that will extend beyond 6.2 (for 6.3 it'd be great to properly support fixed in core blocks)

According to this bit, it sounds like adding a comment stating this is not to backport until we are ready (support fixed in core blocks) could be useful. Thanks for wrangling it.

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants