We still have 2 different mechanisms in place, maybe even 3:
* We simplify the CSS selector when we know a popup type is
disabled, and it's impossible an anonymous user can enable it
at run-time.
* We create that "initiallyEnabled" map that allows anonymous
users to toggle the individual popup types at run-time.
* This map is also used to check if the footer link should be
shown.
* There is also a wgPopupsReferencePreviews global that acts as
a "kill switch". However, this is not a pure feature flag,
but incorporates the user setting for registered users. This
is currently partly redundant (checking
`mw.user.options.get( 'popupsreferencepreviews' )` does the
same) and can be removed later when the feature flag is not
needed any more.
The footer link currently acts odd because anonymous users are
unable to enable ReferencePreviews, but get the footer link.
This patch introduces a 3-state model:
* `true` acts as before.
* `false` means a popup type is disabled, but anonymous users
can enable it (i.e. this is the opt-out behavior for anonymous
users).
* `null` means a popup type is not available at run-time, for
nobody. Anonymous users can't do anything about this.
Registered users must leave the page and change a setting.
Bug: T277640
Change-Id: Id8d1396c09cf0f706034a66f9cd3c880a8b33df8
This patch removes "optional" from the package-lock.json, which is a property introduced in npm v6.6+, while our nvm specifies npm v6.4.1
Change-Id: I43983688d8f26e68b454e42410760b270dced48c
* That `this.user` is unused.
* Some tests missed a module name. This means they are reported
as part of the previous module. While this is purely cosmetic,
it's confusing to see the wrong module name in the report.
Change-Id: I73915d3c4fd9a03bda1ddc8dff6dd5539113c3cd
An "advanced" option was first introduced in 2014 via patch I374805e
(originally named "monitor-or-edit", renamed via patch I7b4f6d2).
The isNavPopupsEnabled() function was added in 2016 via patch
Ic660f48.
The code that disables the extension entirely the moment the NavPopups
gadget is enabled was added in 2017 via patch Ia474b1b (T151058) and
patch Ia837816 (T160081).
As of now, the "advanced" option can only be seen in an extreme edge
case:
* Only for anonymous users.
* Only if NavPopups is enabled by default for anonymous users.
* Only if the $wgPopupsConflictingNavPopupsGadgetName setting is
misconfigured.
* … or if NavPopups is not a gadget in the first place, but e.g.
loaded via Common.js.
In this situation the settings dialog opens with all *3* options. This
is broken for several reasons:
* The "simple" option enables the extension, but doesn't disable
NavPopups. Both trigger, resulting in both popups being displayed
the same time.
* Since "simple" is the default, this bogus behavior is the default
for anonymous users.
* The "off" option doesn't stick. Every time the settings dialog opens
"advanced" is checked instead.
* "Off" can't work anyway. There is no code to disable the gadget.
* Only the "advanced" option "works", but more by accident.
It's unclear how to fix this:
* There is no code that does anything with the "advanced" option. It's
not even stored. The behavior of the option is identical to "off".
* The code appears as if "advanced" was meant to be shown instead of
"off". I.e. anonymous users can only choose one of the popups, but
not disable both. But there is no code to hide the "off" option.
* The bug when both popups are displayed was fixed in 2017 via an
entirely different mechanism. Re-introducing "advanced" does not
only mean duplication, it's unclear how the 2 mechanisms are meant
to work together.
It really, really feels like this was just forgotten.
Bug: T278949
Change-Id: Iab21f3a649a5b2f19ebb0d0dbb45ce1450c65678
… as well as in one place in production code. The motivation
for this change is to make the code easier and faster to read.
There is a little bit of duplication in the test setup now.
But I would like to argue this is a good thing. The values are
rather trivial. The difference (or absense of a difference) is
much easier to see now.
Change-Id: I9aa95b59f0c45ea7c9257970e2fcdba3a000d234
This patch does nothing but rename a pair of variables:
"prevState/state" becomes "oldState/newState". Reasoning:
1. The abbreviated "prev" is confusing, especially because we
are in a codebase that is all about "previews".
2. We are in a context that is all about a state **change**.
Change listeners get notified about the change from one state
to another. While it would be possible to stick to the already
mentioned "previous/current" terminology, I find the word
"current" confusing. What is "current" in this context? Did
the state already change? Am I notified about a change that is
**going** to happen or already happened? Is this even relevant?
I don't think it is. Therefor "old/new".
Another possibility is "previous/next".
Change-Id: Id886e1a095967fe86fb9021f59e335c62da8994e
The nextState() function was not able to understand updates that
are deeper than a single level. Example:
nextState( state, { pagePreviews: { enabled: true } } )
Before, this would replace whatever was in the "pagePreviews"
property with { enabled: true }, but not merge it. In some places
a single level of recursion was done manually because of this.
This can be removed now.
This is done in preparation for splitting the "enabled" flag into
separate ones for each popup type.
Bug: T277639
Change-Id: I35911c18018ba7cd1633a4c882b978656c3fee36
Note how getIsEnabled() is documented: "if the user hasn't
previously enabled or disabled Page Previews […] then they
are treated as if they have enabled them."
In other words: The idea that the default should be true is
encoded twice in this code. This is just not necessary. We can
remove one without loosing anything.
Motivtion: Simplifying the code and reducing the package size.
Since the code fundamentally depends on this default value
anyway, we can clear the users localStorage when they decide
to go back to the default – instead of storing a "1" which
does the same as the default.
Change-Id: I2814a1e9269979918609162a508eeee6944d9e52
The main motivation here is to dramatically reduce the number
of places that use the same property name "enabled" for values
on different objects (e.g. "state", "actions", and "updates"
are all different things) with slightly different meanings. I
tried hard to come up with names that reflect better what each
meaning is.
Bug: T277639
Change-Id: Ie766259793f716262e3d4622ca55156d11f4842c
… instead of only 2 hard-coded nesting levels, as it was before.
This is done in preparation for splitting the "enabled" flag into
separate ones for each popup type. This patch here doesn't change
any behavior or internal representation of the states.
Bug: T277639
Change-Id: Icad669d1c9675ad6de22f478e254debe5d1936d7
Add message, description, extension for title. Update createPagePreview, renderPagePreview methods to add title attribute to settings gear icon. Add test for title attribute. Increase maxSize, maxAssetSize, maxEntrypointSize. Add compiled js files.
Bug: T274887
Change-Id: Ibb29deb3418569d8283b954b4b22074423e78bda
Links at the end of reference previews were covered by the footer
which made them unclickable.
Bug: T277364
Change-Id: I66313a282f24fd45804574577d8e071572de92b6
The bottom margin appears significantly smaller in popups that
open to the top. This is because the triangle ("pointer") falls
within this margin, not outside of it.
This is only temporary. Later the settings icon will always be
there, and the issue is gone.
Bug: T276200
Change-Id: Ic0f0447b8fbd3e6ff7035bdc9380c19a236f091c
To reduce size of code added to the <head> and increase performance.
The increased bundlesize is still less than the size spared bytes in
ResourceLoaderGetConfigVars. - But nevertheless the main gain is loading
less in the <head> anyways.
To avoid further complexity in the code, the bitmask is converted to
the according config setting early on instead of adding checks on the
bitmask all over the place.
Tests will be added in follow ups.
Bug: T276716
Change-Id: Ib4f82bed58295b25f0a41cb37e36244e45f16317
