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

Theme JSON: Clarify status of fixed position opt-in in appearance tools #48660

Merged

Conversation

andrewserong
Copy link
Contributor

What?

Fixes: #48635

Add a comment to the Theme JSON class clarifying the current status of the position.fixed opt-in within appearance tools.

Why?

As raised in #48635, without a comment, it isn't clear why there's a discrepancy between the core code and the Gutenberg code for this particular opt-in.

As discussed in #48635 (comment) and WordPress/wordpress-develop#3973 (comment), Gutenberg should be slightly more permissive than core here, so that it's easier to test subsequent explorations in adding fixed to the Group block, but this wasn't desired in core. In core, fixed is a valid setting, but it needs to be opted-in intentionally so that it isn't accidentally exposed anywhere.

How?

Add a comment to the Theme JSON class explaining the current status of the position.fixed opt-in. This uses the same // BEGIN EXPERIMENTAL and // END EXPERIMENTAL comments I've seen used elsewhere.

Testing Instructions

  1. Proofread the comments.
  2. (Optional) Load up the site editor or post editor with TT3 theme, and check that you can still access the Position panel and set Sticky (this just checks that I didn't add a typo or anything to break the PHP file)

@andrewserong andrewserong added [Type] Code Quality Issues or PRs that relate to code quality Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Mar 1, 2023
@andrewserong andrewserong requested a review from oandregal March 1, 2023 23:01
@andrewserong andrewserong self-assigned this Mar 1, 2023
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for adding the detailed comment @andrewserong 👍

I think it helps explain why position.fixed is being added to the appearance tools opt-in within Gutenberg only and clearly marks its inclusion as experimental. There's one small typo in it though.

The sticky position support continues to function for me as well.

Screenshot 2023-03-02 at 11 18 04 am

lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved
Co-authored-by: Aaron Robertshaw <[email protected]>
@andrewserong
Copy link
Contributor Author

Thanks for the review and for spotting the typo @aaronrobertshaw!

@andrewserong
Copy link
Contributor Author

Thanks for the reviews!

@andrewserong andrewserong merged commit 51e25a5 into trunk Mar 2, 2023
@andrewserong andrewserong deleted the update/clarify-fixed-position-in-theme-json-class branch March 2, 2023 21:58
@github-actions github-actions bot added this to the Gutenberg 15.4 milestone Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WP_Theme_JSON classes: clarify status of position.fixed
3 participants