moderntribe / square-one

Modern Tribe's legacy WordPress framework for the classic editor.
GNU General Public License v2.0
89 stars 20 forks source link

RFC: Move General Settings to the Customizer #1012

Closed dpellenwood closed 2 years ago

dpellenwood commented 2 years ago

What does this do/fix?

Moves the "General Settings" fields from the admin > Settings menu into the Customizer and sets up a basic system for adding additional Customizer sections / fields in preparation for the Site Footer & Masthead settings.

The goal with this change is to consolidate our site settings into the Customizer rather than mixed between the generic "General Settings" page--which is redundant, often a mix of random settings, and not always clear to clients--and the Customizer.

While this isn't required for the upcoming masthead & footer work, I generally find myself or another dev doing this on each new project anyway, so I feel it's a worthwhile addition to SquareOne.

QA

Screenshots/video:

Tests

Does this have tests?

dpellenwood commented 2 years ago

Looks like I could use a hand understanding the specific PHPStan error I'm getting. 😬

defunctl commented 2 years ago

@dpellenwood is it a good idea to utilize the customizer now that WordPress hides it on block themes?

https://make.wordpress.org/core/2022/01/07/state-of-the-customizer-with-block-themes-in-wordpress-5-9/

This is already a pretty bad UX decision (removing a common menu item) and the WP core team seems to be discussing the future of the customizer all together. Seems like our original ACF settings might be a better long term solution no matter which path core takes?

If we really want to use the customizer, then there are some dev patterns we should implement in place here for BE devs which we can discuss on slack to better align with utilizing our DI container and provide contracts/interfaces for them to implement.

dpellenwood commented 2 years ago

Thanks @defunctl ,

@dpellenwood is it a good idea to utilize the customizer now that WordPress hides it on block themes?

I agree, when we move to full site editing/theme.json integration, then the settings shown here will end up somewhere else. (And some of them will probably go away completely in favor of template parts.) Until that time, I'd like to be consistent with where we put site-wide settings, and using the Customizer for this is still the most commonly recognized approach for WordPress (non-FSE) themes. It also provides customers the ability to preview these changes in real-time versus having to save a change and make it "live" without being able to review.

This is already a pretty bad UX decision (removing a common menu item)

Hmm... I don't follow this comment. The WordPress Settings > General page is not changing. I'm removing our custom ACF options page which appears in the menu at Setting > General Settings.

then there are some dev patterns we should implement in place here for BE devs which we can discuss on slack

Agreed. Let me know when you have some time to point me in the right direction here. I assumed the BE portions would need a bit of TLC as I only pulled code from a few older projects to get this working.

defunctl commented 2 years ago

@dpellenwood

RE: UX I meant the bad UX was that WordPress core will sometimes show and hide the customizer dashboard menu item based on some context around the theme, nothing to do with this PR specifically!

Just one day it might not be there and our clients/users may be used to it. Instead, they should have just kept the menu item and redirected to the full site editor with an explanation if that's the long term plan that the WP core team is doing.

One thing to point out that doesn't really matter is just with Google Tag Manager, there is nothing to preview, so kind of weird UX for that as the screen would flash etc but nothing of course would actually visually change.

RE: architecture The quickest way to do this is I'll PR the changes to your branch here and then happy to explain all the "why's" at any time after that as it's easier to reference code directly rather than talk about something that doesn't exist yet :)

defunctl commented 2 years ago

Hey @dpellenwood,

Working on a client project and we just got a request to create a global setting to set a default color site wide, which is using a centrally configured/shared ACF Swatch field.

As that project still has the Global Settings, this is a pretty simple update.

However, If the Global Settings page is removed, where are you thinking the best place to put global ACF configuration like this would be, or really any situation where an ACF field needs to be used or a custom Customizer field is just too complex to make/would create duplicate logic?

dpellenwood commented 2 years ago

Hey @dpellenwood,

Working on a client project and we just got a request to create a global setting to set a default color site wide, which is using a centrally configured/shared ACF Swatch field.

As that project still has the Global Settings, this is a pretty simple update.

However, If the Global Settings page is removed, where are you thinking the best place to put global ACF configuration like this would be, or really any situation where an ACF field needs to be used or a custom Customizer field is just too complex to make/would create duplicate logic?

Yep. This is a great question, @defunctl . My optimal answer is that this is exactly the type of option/setting which should be in the Customizer or in a location where it can be previewed and reviewed visually like the Customizer provides. Imho, a client shouldn't be saving settings which affect the fontend of the site without being able to see them first. We've added swatch fields to the customizer on several projects in the passed, but of course it's a custom solution b/c you can't use ACF fields in the customizer.

Practically speaking, the Customizer's field API is garbage and won't ever be improved, so don't have a good option for complex fields such as color swatches, links (combined label + URL, preferably w/ a dialog to choose from the WP post selector), etc. Which is a bummer. It'd be sweet if ACF supported the customizer.

In the block-theme world, the site Styles editor is probably where this type of setting should end up, but we're a long way from that in SquareOne.

On a side-note, Even WP's latest theme, TwentyTwentyTwo, still utilizes the Customizer; albeit, with a blurb at the top about trying the (beta) Site Styles editor instead.

dpellenwood commented 2 years ago

Closing this PR after a few conversations re: Customizer versus ACF-based settings pages. I'm going to create a new PR with a tabbed "Appearance > [Theme Name] Options" page to replace the existing "Setting > General Settings" and to introduce the necessary fields for the Footer, et al. We loose the preview abilities, but gain the much more robust fields API from ACF going this route.

In the future we can explore converting some of the settings over to FSE template parts/features, but we want to get this final Footer & Masthead pieces of SquareOne complete now before we start exploring those more complex topics.