Site-kit-wp: Enable settings view and settings edit component registration

[felixarntz]

Feature Description

We need to actually allow registration and usage of SettingsView and SettingsEdit components for modules.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• The registerModule action of the core/modules store should be modified:
  
  
  
  • The settingsComponent argument should be removed.
  
  
  • Instead, optional settingsViewComponent and settingsEditComponent arguments should be introduced, which should be React components for the module's view and edit component respectively. There should not be any default (i.e. undefined).
  
  
• The DefaultModuleSettings component should be removed.
• Every module that currently uses the googlesitekit.ModuleSettingsDetails-{$slug} filter should instead include a dispatch call to registerModule, passing its SettingsView and SettingsEdit component as applicable (some modules have both, some modules have just the view, other modules may have neither).
• A new functional hook-based component SettingsRenderer should be introduced:
  
  
  
  • It should receive slug (string), isOpen (bool), and isEditing (bool) props.
  
  
  • It should basically mirror the implementation of today's SettingsMain component of every module.
  
  
  • It should assume modules/${ slug } as the module's store name for now.
  
  
  • It should retrieve the module's settingsViewComponent and settingsEditComponent from core/modules datastore's getModule selector.
  
  
• The SettingsModule component should be modified:
  
  
  
  • It should be wrapped in the withSelect HOC, to receive the registered module's data via the datastore. For now, it should only pass the hasSettings prop to the inner component, based on whether there is a moduleData.settingsEditComponent (i.e. hasSettings = !! moduleData.settingsEditComponent).
  
  
  • The googlesitekit.ModuleSettingsDetails-{$slug} filter should be removed, and instead it should render the above new SettingsRenderer component.
  
  
  • The component should not be further refactored, or as little as possible.
  
  
• The now unused ModuleSettingsDetails component should be removed.

Implementation Brief

• Move assets/js/modules/analytics/components/settings/SettingsMain.js to assets/js/components/settings/SettingsRenderer.js
  
  
  
  • Rename to SettingsRenderer.js
  
  
  • Update it to accept a slug prop and use a dynamic STORE_NAME defined as modules/${ slug } (see https://github.com/google/site-kit-wp/issues/1623#issuecomment-708328766)
    
    _STORE_NAME should be changed to storeName now as it is no longer a constant_
  
  
  • Select SettingsEdit and SettingsView from the module, falling back to a function that returns null so that they will safely render nothing if they don't exist in the store
  
  
• Update SettingsModule (assets/js/components/settings/settings-module.js) to be wrapped with Data.withSelect and pass through the hasSettings prop
  
  
  
  • Remove googlesitekit.ModuleSettingsDetails-${ slug } filter
  
  
  • Replace FilteredModuleSettingsDetails with SettingsRenderer passing along the same props, as well as slug
  
  
  • Remove hasSettings prop passed from SettingsModules
  
  
• Update all registerModule calls for all core modules to provide their SettingsView and SettingsEdit components to the settingsViewComponent and settingsEditComponent respectively (where applicable)
  
  
  
  • Use invariant to require that these values are functions (if provided) using component && ReactIs.isValidElementType( component )
    
    Add react-is to dependencies (already required by multiple packages, and dependency of prop-types)
  
  
• Remove the settingsComponent key from moduleDefaults in assets/js/googlesitekit/modules/datastore/modules.js
• Remove all calls to (add|remove|removeAll)Filters( 'googlesitekit.ModuleSettingsDetails- everywhere
  
  
  
  • Module index.js modules
  
  
• Remove assets/js/googlesitekit/modules/components/DefaultModuleSettings.js
• Remove assets/js/components/settings/module-settings-details.js
• Remove assets/js/modules/*/components/settings/SettingsMain.js and references

Test Coverage

Jest

• Add a assets/js/components/settings/SettingsRenderer.test.js using a basic/dummy test module (not using analytics, tagmanager, etc)
  
  
  
  • Test the behavior of the various props on the rendered output, as well as covering the settings rollback behavior
  
  
• Update assets/js/googlesitekit/modules/datastore/modules.test.js to cover the behavior of the changes to registerModule
• Remove assets/js/modules/*/components/settings/SettingsMain.test.js

Storybook

• Update createLegacySettingsWrapper
  
  
  
  • Remove googlesitekit.ModuleSettingsDetails-${ slug } filter
  
  
  • Remove the hasSettings prop to SettingsModule as this is now provided by the HOC
  
  
• Update module-*-settings.stories.js to dispatch( core/modules ).registerModule(...) with the settings components provided on the fresh registry in the addDecorator function
  
  
  
  • E.g. https://github.com/google/site-kit-wp/blob/099cac9a45e52cb90bc4473a68f37eb7f6f45666/stories/module-adsense-settings.stories.js#L96-L99
  
  

Visual Regression Changes

• N/A

QA Brief

• Check module settings components in Storybook or WP with React devtools to see that all module settings components are rendered by a SettingsRenderer component.

Changelog entry

• Allow registering a settingsViewComponent and settingsEditComponent when calling the registerModule action on the core/modules store.

[felixarntz]

@tofumatt Also for your review (follow-up to #1622), let's make sure we agree on the ACs (which largely are the IB here) and then go from there.

[tofumatt]

One note: in the IB I changed setSettingsState and getSettingsState to setSettingsDisplayMode and getSettingsDisplayMode. The former was a bit too vague for me so I wanted to clarify the intention. If you're cool with that we can update the ACs too, but it's just the name I'm proposing changing, not the functionality 😄

Much of the ACs are copied as they're more/less IBs, and sound good to me 🙂

---

I've haven't modified the IB here but I wanted to make a note around the "locked" state we use for modules. Now that we store unsaved state in Redux, and can better load/unload editing screens, I think we should rethink the "locked" UX.

If anything, setting other modules _state_ to "locked" is fine, but instead of forcing a user to close their current module before switching, we should _confirm_ they want to exit editing without saving changes and then just let them.

Anyway, just a point to make really; doesn't affect the data store implementation.

[felixarntz]

@tofumatt That change sounds good, I'll update this in the ACs too.

Regarding "locked" display mode, that's a fair point for consideration in the future. Although, I'm not sure I understand the technical concern here - when you're editing, you can't go to another module's settings, so you would never lock a settings panel where you were just editing.

IB ✅

[eugene-manuilov]

@felixarntz, @tofumatt, @aaemnnosttv this is ready for approach review. I have pushed everything to the feature/1623-implement-public-components-for-usage-in-module-settings-components which you can pull and check on your local. It is also available in the #1904 if you want to leave comments or request changes.

As an example, I have reworked Analytics module to work with the new settings components: https://github.com/google/site-kit-wp/blob/325ac41df980698533cf2bb46478d7ec3c63dfa9/assets/js/modules/analytics/components/settings/SettingsMain.js#L59-L68

Let me know what you think.

[tofumatt]

I didn't do too thorough of a code review on the PR yet as I expect more changes based on feedback, but the approach above (and in the PR) seems good to me 😄

One note is I agree with @felixarntz's comment here: https://github.com/google/site-kit-wp/pull/1904/files#r471855142. DefaultModuleSettings seems like the wrong sort of name for this component—Felix's suggestion of SimpleModuleSettings makes sense to me, especially as it leaves open a future, more niche component that had "full control" over the settings component rendered.

But 👍 to this approach. Let me know if you need anything else from me before continuing, but I think I like the approach 🙂

[felixarntz]

After another long conversation about this issue, I'll move this back to ACs. This issue has been too undefined overall - with all the alternative approaches suggested after the initial PR had already started, we've ended in some scope creep and still have no clear approach to go with. Let's regroup and decide on something next week and then pick this up with a new PR in Sprint 34.

cc @aaemnnosttv @tofumatt @eugene-manuilov

[aaemnnosttv]

• Instead of relying on a hasSettings prop, hasSettings should be set based on whether there is a moduleData.settingsEditComponent.
• The component should not be further refactored, or as little as possible.

@felixarntz in the interest of minimizing the changes here, I would suggest we continue to pass hasSettings but pass it from withSelect based on the presence of the component.

Also, to avoid the temptation of refactoring other module-data related pieces like getModulesData use, I think we should pass the module components through as dedicated individual props, rather than passing through all of the selected moduleData from getModule().

---

I think the last remaining detail to clarify is around what happens to SettingsMain which is currently the component which is filtered in for all modules. This component is pretty much the same in all cases and is mostly used as a switch to control which of the two components to render, if anything. Apart from this though, the component has a useEffect hook which conditionally rolls back the settings which cannot be simply moved into one of the other two child components. To preserve this functionality, I think we can get away with keeping a common SettingsMain component to use as a wrapper in SettingsModule. Then we actually no longer need to use withSelect and SettingsMain can make these selections internally and pass anything else needed through as props. The problem then is where the store name will come from as this is needed to select things like haveSettingsChanged 🤔 Perhaps we need to add a storeName parameter to registerModule ?

[felixarntz]

@aaemnnosttv

@felixarntz in the interest of minimizing the changes here, I would suggest we continue to pass hasSettings but pass it from withSelect based on the presence of the component.

I don't see really any additional complexity from getting rid of it as a prop. The component receives the edit component anyway, and hasSettings is only used in the render method, so it could become a single assignment like hasSettings = !! settingsEditComponent.

Also, to avoid the temptation of refactoring other module-data related pieces like getModulesData use, I think we should pass the module components through as dedicated individual props, rather than passing through all of the selected moduleData from getModule().

Do you think this will make the task overly complex? I think getting the data from a module prop shouldn't be more work than changing some destructuring assignments to be based on props.module instead of props. All values that are on props.module should come from there, all other values (like provides) should remain a prop. I mostly want to be conscious of not changing _any_ logic or not migrating to use hooks in any way, but reassigning values from another location should be okay IMO.

I think the last remaining detail to clarify is around what happens to SettingsMain which is currently the component which is filtered in for all modules. This component is pretty much the same in all cases and is mostly used as a switch to control which of the two components to render, if anything. Apart from this though, the component has a useEffect hook which conditionally rolls back the settings which cannot be simply moved into one of the other two child components. To preserve this functionality, I think we can get away with keeping a common SettingsMain component to use as a wrapper in SettingsModule. Then we actually no longer need to use withSelect and SettingsMain can make these selections internally and pass anything else needed through as props.

That's a good point. I think that we could add a component that solely takes care of the useEffect hook, this way we can keep the settingsViewComponent and settingEditComponent unaffected and put it right into SettingsModule where I think this makes most sense. The new component (e.g. SettingsRollbackTool or whatever) would be a temporary measure only used until we refactor SettingsModule to become a functional hooks component, at which point it would become part of it.

The problem then is where the store name will come from as this is needed to select things like haveSettingsChanged 🤔 Perhaps we need to add a storeName parameter to registerModule ?

Let's not worry about this for now and just assume modules/${ slug } for the store name. It makes sense for this to be a registerModule argument, but I think for now we can rely on our convention.

[felixarntz]

@aaemnnosttv I've updated ACs based on my above response, lmk your thoughts.

[aaemnnosttv]

I'm not sure about extracting the rollback effect into its own component to be rendered adjacent to the others. It would probably work, but I think we can make this a cleaner refactor by sticking a bit closer to how things are today.

We can simplify things even further with almost no changes needed to SettingsModule by more/less converting SettingsMain into a SettingsRenderer that would be responsible for selecting the registered components from the module and rendering them the same as today, including the rollback effect. The only difference is that it would accept the module slug in order for the same component to be used for all modules.

That would reduce reduce the changes to SettingsModule to only a handful of lines

diff --git a/assets/js/components/settings/settings-module.js b/assets/js/components/settings/settings-module.js
index 99e6d04bd..b2dad12a2 100644
--- a/assets/js/components/settings/settings-module.js
+++ b/assets/js/components/settings/settings-module.js
@@ -198,10 +198,6 @@ class SettingsModule extends Component {
        const subtitle = sprintf( __( 'By disconnecting the %s module from Site Kit, you will no longer have access to:', 'google-site-kit' ), name );

        const isSavingModule = isSaving === `${ slug }-module`;
-       // Disabled because this rule doesn't acknowledge our use of the variable
-       // as a component in JSX.
-       // eslint-disable-next-line @wordpress/no-unused-vars-before-return
-       const FilteredModuleSettingsDetails = withFilters( `googlesitekit.ModuleSettingsDetails-${ slug }` )( ModuleSettingsDetails );

        // Disable other modules during editing
        const modulesBeingEdited = filter( isEditing, ( module ) => module );
@@ -336,7 +332,7 @@ class SettingsModule extends Component {
                                                    mdc-layout-grid__cell
                                                    mdc-layout-grid__cell--span-12
                                                ">
-                                           <FilteredModuleSettingsDetails module={ moduleKey } isEditing={ isEditing[ moduleKey ] } isOpen={ isOpen } />
+                                           <SettingsRenderer slug={ slug } isEditing={ isEditing[ moduleKey ] } isOpen={ isOpen } />
                                        </div>
                                    </Fragment>
                                    }
@@ -501,4 +497,8 @@ SettingsModule.defaultProps = {
    setupComplete: false,
 };

-export default SettingsModule;
+export default withSelect( ( select, { slug } ) => {
+   return {
+       hasSettings: !! select( CORE_MODULES ).getModule( slug )?.settingsEditComponent,
+   };
+} )( SettingsModule );

Then SettingsRenderer would basically be a SettingsMain with the following changes

diff --git a/assets/js/modules/analytics/components/settings/SettingsMain.js b/assets/js/modules/analytics/components/settings/SettingsMain.js
index 895431fd5..7097f7e95 100644
--- a/assets/js/modules/analytics/components/settings/SettingsMain.js
+++ b/assets/js/modules/analytics/components/settings/SettingsMain.js
@@ -25,14 +25,14 @@ import { useEffect } from '@wordpress/element';
  * Internal dependencies
  */
 import Data from 'googlesitekit-data';
-import SettingsEdit from './SettingsEdit';
-import SettingsView from './SettingsView';
-import { STORE_NAME } from '../../datastore/constants';
 const { useSelect, useDispatch } = Data;

-export default function SettingsMain( { isOpen, isEditing } ) {
+export default function SettingsRenderer( { slug, isOpen, isEditing } ) {
+   const STORE_NAME = `modules/${ slug }`;
    const isDoingSubmitChanges = useSelect( ( select ) => select( STORE_NAME ).isDoingSubmitChanges() );
    const haveSettingsChanged = useSelect( ( select ) => select( STORE_NAME ).haveSettingsChanged() );
+   const SettingsEdit = useSelect( ( select ) => select( CORE_MODULES ).getModule( slug )?.settingsEditComponent || null );
+   const SettingsView = useSelect( ( select ) => select( CORE_MODULES ).getModule( slug )?.settingsViewComponent || null );
    // Rollback any temporary selections to saved values if settings have changed and no longer editing.
    const { rollbackSettings } = useDispatch( STORE_NAME );
    useEffect( () => {

It's also worth noting that ModuleSettingsDetails (placeholder component for () => null) is no longer needed and can be removed here.

What do you think? I think that covers everything and would be the simplest change to make.

[felixarntz]

@aaemnnosttv SGTM, I've updated the ACs.

[aaemnnosttv]

@felixarntz looks great, moving forward to IB 👍

[felixarntz]

@aaemnnosttv IB mostly LGTM, just one additional detail: We should probably remove hasSettings from being passed to SettingModule in SettingsModules, since that prop is now internally handled via the HOC.

[aaemnnosttv]

@felixarntz IB updated.

[felixarntz]

IB ✅

[tofumatt]

QA ✅

Everything being rendered properly here: