bigbite / themer

GNU General Public License v2.0
2 stars 0 forks source link

Warning when no theme.json exists #54

Closed squarewave17 closed 5 months ago

squarewave17 commented 7 months ago

Expected Behavior

If no theme.json exists or if the plugin is installed on a classic theme, a warning should be displayed to the user. Potentially, the option could be given to initialise a default theme.json

Actual Behavior

When no theme.json is present, the UI hangs in it's loading state

Step-by-step reproduction instructions

  1. Install and activate twenty twenty-two theme
  2. Open the UI
  3. Hangs

Screenshots, screen recordings, code snippets

Screenshot on 2024-04-23 at 15-01-44

UPDATE

On further inspection, it seems the main issue here is that Themer can't use theme.json properly until an edit is made and saved to the database. The fault occurs on any theme.

A possible solution here is to use wp_get_global_settings() as our source of truth for style data.

This function is capable of using all three style sources at once - core, theme and user in the precedence intended by Wordpress.

g-elwell commented 7 months ago

Thanks for adding the issue @squarewave17

This is making me wonder whether a theme.json file is actually required for themer to work. Theme customisations are stored in the database, so if we stop assuming the file is there to begin with, can we allow users to apply styles through the UI and save them, without actually needing the file at all?

However, I know WordPress uses the presence of a theme.json file as an indicator that a theme is an FSE/block theme, so we might hit some issues there, and the easier option of prompting the user to add the file if it's missing might be safer.

Since we're looking to release a v1 of this plugin soon, I would recommend we just go with the error message approach for now. We definitely need to avoid showing a blank screen in any situation.

squarewave17 commented 7 months ago

I can confirm theme.json file is not required for a block theme, only style.css and and index.html inside of a template folder.

There is a default theme.json located in wp-includes which is used when no theme.json is included in the theme, so looking to use that as a fallback might be useful.

An error message is certainly required. Happy to take this on if you like @g-elwell ?

g-elwell commented 7 months ago

An error message is certainly required. Happy to take this on if you like @g-elwell ?

Yeah of course! I'm not sure if it's documented anywhere (sorry) but we have a release/1.0.0 branch in progress currently to branch off / PR against.

balw commented 5 months ago

Just adding this here for who ever picks this up, not sure if this was tested on the release branch or if there is now an issue with this implementation.

https://github.com/bigbite/themer/pull/16

thatisrich commented 5 months ago

Taking a look at this issue it looks like there may be two approaches to resolving, one of which is already up and running and the other which tackles a deeper WordPress recommendation.

@g-elwell @jonnywatersbb For v1.0.0, do the current error messages (@balw's implementation) cover off this particular issue or would it be preferable to tackle the deeper issue (WP_Theme_JSON_Resolver) prior to launch?

A demo of the current implementation can be seen here which covers a missing theme.json and an invalid theme.json.

jonnywatersbb commented 5 months ago

Maybe need @g-elwell to clarify but I believe #16 may cover this ticket and it can be closed?

59 can be left as a separate issue, which I believe @squarewave17 is currently looking into?

squarewave17 commented 5 months ago

@thatisrich this ticket can be ignored for now. I should have probably removed this as it's quite confusing.

59 is a new version of this ticket.

What we really need to do is just check that the theme is FSE or not, as every FSE them WILL have a theme.json even if there isn't one in the theme folder. There is a default theme.json in wp-includes so providing we use the new functions outlined in #59 WP will take care of the theme.json loading and precedence. It transpired that the error in this ticket comes from the way we are trying to load theme.json, not because of it's presence.

g-elwell commented 5 months ago

I agree, we can close this off as it's essentially a duplicate of #59 at this point.