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 Enable settings view and settings edit component registration #1623 (comment))
    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.

    site-kit-wp/stories/module-adsense-settings.stories.js

    Lines 96 to 99 in 099cac9

    	storiesOf( 'AdSense Module/Settings', module )
    	.addDecorator( ( storyFn ) => {
    	const registry = createTestRegistry();
    	registry.dispatch( STORE_NAME ).receiveGetSettings( {} );

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 ✅

[felixarntz]

IB ✅

[tofumatt]

QA ✅

Everything being rendered properly here: