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

[BBT-121] Manage all styles #45

Merged
merged 8 commits into from
Dec 5, 2023

Conversation

g-elwell
Copy link
Member

@g-elwell g-elwell commented Dec 4, 2023

Description

Fixes BBT-121 - Allows the addition and editing of style attributes even when they are not present in the original theme.json file

This PR builds on previous work on style components, and simply removes their conditional rendering so that they are always present on every block/element/pseudo-element.

There were a few adjustments to be made to accommodate undefined variables, but once addressed the components work as expected, allowing users to save and edit styles.

Change Log

  • Unconditionally render all style components
  • Fix undefined variable errors
  • In StylesColor - render colour pickers for "background" and "text" rather than checking for presence in the theme

Steps to test

  • Load themer with a blank theme.json file (may need to contain {})
  • Add and edit styles of various types across multiple blocks/elements
  • Attempt to save styles, export theme.json, reset styles, etc.
  • If any crashes are experienced, please share console logs
  • Note that interactive styles (eg hover, focus) will not be visible unless you remove the inert attribute from the preview container (see video)

Screenshots/Videos

Screen.Capture.on.2023-12-04.at.14-53-30.mov

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@g-elwell g-elwell added the enhancement New feature or request label Dec 4, 2023
@g-elwell g-elwell self-assigned this Dec 4, 2023
@g-elwell g-elwell marked this pull request as ready for review December 4, 2023 14:58
@g-elwell g-elwell marked this pull request as draft December 4, 2023 14:58
@g-elwell g-elwell marked this pull request as ready for review December 4, 2023 15:03
Copy link

@Joe-Rouse Joe-Rouse left a comment

Choose a reason for hiding this comment

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

Found an issue with both the padding (shown below) and margin controls on blocks without existing padding/margin styles:

Screen.Recording.2023-12-04.at.16.19.17.mov

Copy link

@Joe-Rouse Joe-Rouse left a comment

Choose a reason for hiding this comment

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

Found an issue where you can't change the font appearance to anything but default/regular/regular italic:

Screen.Recording.2023-12-04.at.16.21.49.mov

@g-elwell g-elwell force-pushed the feat/BBT-121-manage-all-styles branch from 5dff47f to f0b01be Compare December 5, 2023 08:54
@g-elwell
Copy link
Member Author

g-elwell commented Dec 5, 2023

Thanks @Joe-Rouse - I've fixed both of those issues now and re-checked all inputs, nothing else is coming up for me.

The padding/margin one was similar to the previous issues, I had just missed it.

The font updating one was a little different, perhaps it wasn't showing up until this refactor, but we were attempting to update the config object twice during the same action which caused only one of the values to update. Refactoring the onChange handler to accept an object rather than a value and a single key prevents this, and with hindsight may be a better approach overall.

@Joe-Rouse
Copy link

@g-elwell Looks pretty good to me now. One thing I was wondering before approving though, do you know why some of these blocks aren't greyed out even when using an empty theme.json ({})?

Screenshot 2023-12-05 at 09 51 45

I only get it on those two and the columns block.

@g-elwell
Copy link
Member Author

g-elwell commented Dec 5, 2023

@g-elwell Looks pretty good to me now. One thing I was wondering before approving though, do you know why some of these blocks aren't greyed out even when using an empty theme.json ({})?

Screenshot 2023-12-05 at 09 51 45 I only get it on those two and the columns block.

Yeah, it looks as though WP hard-codes some base styles even when the theme theme.json file is empty.

https://github.com/WordPress/wordpress-develop/blob/f2c05952c07bf8860a49211854479a5e478069d2/src/wp-includes/class-wp-theme-json-resolver.php#L153

Core theme.json is here: https://github.com/WordPress/wordpress-develop/blob/6.4/src/wp-includes/theme.json

@Joe-Rouse Joe-Rouse self-requested a review December 5, 2023 10:18
Copy link

@Joe-Rouse Joe-Rouse left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested where I can and I can't make it crash anymore. Other features like the theme.json exporter/, reset etc. all still work as expected.

@g-elwell g-elwell merged commit 2de3814 into release/1.0.0 Dec 5, 2023
1 check passed
@g-elwell g-elwell mentioned this pull request Jul 26, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants