mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/Popups synced 2024-11-15 03:34:03 +00:00

joakin efb7b3d872 Wire up saving enabled/disabled in settings dialog

The save action has been implemented, and it is listened to by the
canonical enabled state in the previews reducer, and by the settings
reducer to perform UI changes.

The enabled state of the application has been kept in the preview
reducer as the canonical source of truth. See supporting changes for
documentation about the decision.

Actions:
* Introduce new action SETTINGS_CHANGE with the enabled status
* Trigger that action when clicking Save in the settings dialog

Reducers:
* Listen to SETTINGS_CHANGE in the preview reducer to update enabled
  status
* On the settings reducer
  * Handle the SETTINGS_CHANGE action
  * Add showHelp flag to determine if the help should be showing

Change listeners:
* Switch to compare past vs present changes in the implementation
* Handle showing and hiding the help

Supporting changes:
* On the rendered settings dialog:
  * Change #hide to actually just hide and remove legacy if statement
  * Add #toggleHelp method to show or hide the help dialog
* Add doc/adr/0003-keep-enabled-state-only-in-preview-reducer.md to
  support the decision of making the saveSettings action creator return
  a Redux.Thunk and keeping the enabled state just in the preview
  reducer.
  * Add NB to actions#saveSettings explaining and linking to the
    document

Follow commits soon:
* Persist the settings change to local storage when it changes, and
  unify with the preview change listener

Change-Id: I80dc5f29fbe6286f2e3e3b50d909894bc5041ccd

2016-12-14 14:35:59 +00:00

3.3 KiB

Raw Blame History

3. Keep enabled state only in preview reducer

Date: 14/12/2016

Status

Accepted

Context

Discussed by Sam Smith and Joaquin Oltra.

There is global state for determining if the previews are enabled or disabled. It lives in the preview reducer as the enabled key.

As part of implementing the settings, it was noticed that:

When the settings are saved in the UI dialog,
And a SETTINGS_CHANGE action is triggered with the new enabled state,
Then the settings reducer (reducers/settings.js) also needs to know about the previous enabled state to determine if it:
- should ignore the change
- hide the UI
- or show the help if the user is disabling

Given the enabled state was kept in the preview reducer, there are several options considered:

Add an enabled property to the settings reducer, duplicating that part of the state in both preview and settings reducers.

Pros
- saveSettings action creator remains synchronous
- settings reducer internally contains all it needs to act on actions
Cons
- enabled state, action handling and state toggling, and tests are duplicated in both reducers/preview and reducers/settings
- Maintenance overhead
- Confusion about which of both to use for taking decisions in other parts of the source
- Risk of the enabled flags getting out of sync with future changes

Rely on UI dialog captured state to trigger saveSettings with the current and new state.

Pros
- saveSettings action creator remains synchronous
- settings reducer gets via action all it needs to act on actions
Cons
- enabled state is duplicate in the preview reducer and in the created settings dialog (either as a captured variable, or as DOM state)
- Confusion about which of both to use for taking decisions in other parts of the source
- Risk of the enabled state on the UI getting out of sync with future changes, and triggering stale state with the action resulting in bugs

Keep the enabled flag in the preview reducer as the canonical source of enabled state. Convert saveSettings to a Redux.Thunk, to query global state, and then dispatch current and next state in the action.

Pros
- enabled state exists in just one place in the whole application. No duplication
- settings reducer gets via action all it needs to act on actions
Cons
- Confusion about the use of a Redux.Thunk on a synchronous action creator, where in the docs they are used only for asynchronous action creators

Decision

After code review and discussing the different options and trade-offs, the implementation of 3 was chosen mainly because of the clarity that having one piece of state just in one place brings for understanding the application, and the benefits on maintainability.

Extensive documentation and tests have been written in the action creator and in this document to explain the choice made, which should overcome the cons.

Consequences

The saveSettings action creator is now a Redux.Thunk, which uses getState to query the enabled state in the preview reducer, and adds it to the SETTINGS_CHANGE action as wasEnabled. As such, the settings reducer can act on SETTINGS_CHANGE to perform its business logic.

3.3 KiB Raw Blame History