Add actions and selectors for managing module settings panel state

[felixarntz]

The core/modules store should support managing the state for the settings panels Site Kit Settings.

---

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

Acceptance criteria

• The core/modules store should receive a new selector getModuleSettingsPanelState( slug ) that returns for the module identified by the given slug in which state the panel is (e.g. whether it is closed, in "view" or "edit" mode etc.).
• The core/modules store should receive a new action setModuleSettingsPanelState( slug, value ) that sets the panel state for the module identified by the given slug to the given value.
• The following settings panel states should be possible/valid to be passed to setModuleSettingsPanelState:
  • closed
  • view
  • edit
• There should be a separate locked "meta-state" which cannot be settable via the action, but selectable separately.
• Dedicated selectors should be added for each state:
  • isModuleSettingsPanelOpen( slug )
  • isModuleSettingsPanelClosed( slug )
  • isModuleSettingsPanelEdit( slug )
  • isModuleSettingsPanelLocked( slug )
• The initial value for every module's settings panel should be closed.
• The following rules should be applied when modifying the settings panel state or selecting from it:
  • Only a single module can be viewed/open/edited at a time
  • A module is locked when another module is being edited

Implementation Brief

• Create a new file to house the selectors/actions: assets/js/googlesitekit/modules/datastore/settings-panel.js.
• It should have a state shape as follows where ModuleSlug is a string or null:

  {
      settingsPanel: {
          currentModule: ModuleSlug, // default `null`
          isEditing    : Boolean,    // default `false`
      }
  }

• Add a new selector getModuleSettingsPanelState( slug ) that returns view|closed|edit as a string.
• Add state-specific selectors for each state
  • isModuleSettingsPanelOpen( slug ) – (slug === currentModule)
  • isModuleSettingsPanelClosed( slug ) – (slug !== currentModule)
  • isModuleSettingsPanelEdit( slug ) – (slug === currentModule && isEditing)
  • isModuleSettingsPanelLocked( slug ) – (slug !== currentModule && isEditing)
• Add a new action setModuleSettingsPanelState( slug, value ) that allows setting value to one of: 'view' | 'edit' | 'closed'.
  Trying to set any other value should result in an error.
  • If setting slug to view, set currentModule to slug and isEditing to false
  • If setting slug to edit, set currentModule to slug and isEditing to true
  • If setting slug to closed, set currentModule to null and isEditing to false

Test Coverage

• Create a assets/js/googlesitekit/modules/datastore/settings-panel.test.js
• Add tests for
  • actions.setModuleSettingsPanelState
  • selectors.getModuleSettingsPanelState
  • selectors.isModuleSettingsPanelOpen( slug )
  • selectors.isModuleSettingsPanelClosed( slug )
  • selectors.isModuleSettingsPanelEdit( slug )
  • selectors.isModuleSettingsPanelLocked( slug )

Visual Regression Changes

• None.

QA Brief

• Tests should pass but otherwise there is nothing to directly QA for this issue.

Changelog entry

• Add core/modules actions and selectors for managing module settings panel state.

[felixarntz]

@tofumatt Mostly LGTM, but two things don't satisfy the ACs:

Add a new selector getModuleSettingsPanelState( slug ) that returns the value in state.panelState[ slug ] (or undefined if it's not set)

This should never return undefined as there is no "loading" stage here. The initial value for every settings panel should be closed per the ACs. We could consider returning null if slug is not for a valid module - but any valid module should always return a real value.

Add a new action setModuleSettingsPanelState( slug, value ) that allows setting state.panelState[ slug ] to one of: 'view' | 'locked' | 'closed'.

Per the ACs locked shouldn't be allowed, but edit should.

[tofumatt]

Ah, right on. Fixed! 🙂

[felixarntz]

@tofumatt IB pretty much LGTM, however it's still missing a mention of closed being the default value.

undefined should not be returned.

This is true, but it would be more clear saying that, if state.panelState[ slug ] is undefined and it is a valid module, the function should return closed.

[tofumatt]

Ah, right on. I think using a registry selector for modules that aren't yet defined in the panel state should cover that and create an effective default state—let me know what you think about that approach 😄

[felixarntz]

IB ✅

[ivankruchkoff]

Per #2182 (comment) this file assets/js/googlesitekit/modules/datastore/settings.js is being created in the PR #2265

[aaemnnosttv]

@felixarntz I've updated the ACs (mostly additive for clarity) and IB here per our last conversation. Let me know if anything is out of place or missing here to prevent further unnecessary changes for @ivankruchkoff .

@ivankruchkoff – please take a look and let me know if you have any questions or concerns regarding these changes before resuming.

[felixarntz]

@aaemnnosttv @ivankruchkoff ACs look accurate, I think the modified IB can be a bit simplified:

• Instead of state.settingsPanel.editingModule, I think it would be sufficient to make this a boolean like state.settingsPanel.isEditing. Having the module slug there is redundant, since it must always be the same module slug as in state.settingsPanel.currentModule.
• In the case descriptions, "If setting slug to view and slug is the current module, do nothing" is not fully correct, if state.settingsPanel.editingModule (or the above state.settingsPanel.isEditing) is truthy, it should be unset (or set to false).

[aaemnnosttv]

@felixarntz

• Instead of state.settingsPanel.editingModule, I think it would be sufficient to make this a boolean like state.settingsPanel.isEditing. Having the module slug there is redundant, since it must always be the same module slug as in state.settingsPanel.currentModule.

SGTM 👍 I've updated the IB to reference this name for the state instead.

• In the case descriptions, "If setting slug to view and slug is the current module, do nothing" is not fully correct, if state.settingsPanel.editingModule (or the above state.settingsPanel.isEditing) is truthy, it should be unset (or set to false).

From a semantic perspective, I don't think we should implement a toggling behavior for a "set*" action. If we want to offer that capability from the datastore (which is totally reasonable, and something I had added originally as well), then I think it makes more sense to name the action like toggleSettingsPanelEdit and/or toggleSettingsPanelOpen although we can do without these by using the set action and delegate the toggling to the component.

const isPanelOpen = useSelect( ( select ) => select( 'core/modules' ).isSettingsPanelOpen( slug ) );
const { setSettingsPanelState } = useDispatch( 'core/modules' );
const togglePanel = useCallback( () => {
    setSettingsPanelState( slug, isPanelOpen ? 'closed' : 'view' )
}, [ slug, isPanelOpen ] );

This is quite simple to implement so the question is whether or not it makes sense to build that into the datastore or not?
I've updated the section for how setSettingsPanelState should behave, which is even simpler than before and very straightforward. No objections from me about adding an action to toggle a specific state, but only ask that we do that with a separate action.

[felixarntz]

As discussed last week, we had somewhat a misunderstanding regarding your latest comment @aaemnnosttv - the current IB LGTM!

[aaemnnosttv]

@ivankruchkoff – this one is ready to resume if you haven't already 👍

[ivankruchkoff]

@aaemnnosttv - addressed CR feedback and changed the implementation to match the new IB. Reassigned it back to you as you looked at initially.

[ivankruchkoff]

Updated per CR feedback.

[cole10up]

QA ✅

Quick high level regressions of module settings with all modules connected looks good.

Sending to approval