mirror of
https://gerrit.wikimedia.org/r/mediawiki/extensions/Popups
synced 2024-12-24 13:03:09 +00:00
efb7b3d872
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
79 lines
3.3 KiB
Markdown
79 lines
3.3 KiB
Markdown
# 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:
|
|
|
|
1. 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
|
|
2. 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
|
|
3. 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.
|