As described in the readme but not implemented until now, this patch
enables the skin version to be specified as a URL query parameter. This
is useful for testing both skin versions during development and on wiki,
as well as enabling sharing URLs with a specific skin (Vector) and skin
version (1 or 2).
Obtaining the actual skin version requires tying together three input
sources, WebRequest, User, and Config. It seems simple but it'd be easy
to botch. For this reason, a helper class to correctly interrogate them
and tests are provided.
Bug: T244481
Change-Id: I52d80942b4270c008d4e45050589ed9220255a50
Move all the LESS `z-index` settings to variables.less so it's easy to
conceptualize how UI will be layered. jQuery CSS rules are untouched but
documented.
Also, remove the seemingly redundant re-setting of the `mw-searchButton`
`z-index`:
#mw-searchButton
z-index: 1;
}
This rule was added in 0c77e4f.
The sidebar work will require `z-index` properties and this refactor
eases the comprehension of those changes, as well as prevents the
accrual of more `z-index` technical debt.
Bug: T246419
Change-Id: Ic112a0ee3f701f87432838797be45c6069fdb522
The new template 'index.mustache' drops the Navigation
template partial (component) - instead embedding it in
the master template given its lack of usability.
Note to reviewer: Please ensure index.mustache and
legacy.mustache are identical.
Change-Id: I580fe55ca6aeb35dae8afee41e87172a5a729c7b
It is not the most useful of components and adds an additional layer
of complexity similar to multiple inheritance chains that we find in
Object oriented programming.
I suggest we use index.mustache going forward for laying out the different
components and use components/template partials for reusable components.
Change-Id: I6fd5fe1c3d3826d737ccd8ed5a38890305664876
This adds the same end margin as the portal container making the
opt-out-link more visually pleasing if it overflows its container.
Bug: T243281
Change-Id: I42eb3ec4a18ad9e4f6bcdb451593fec7e6e4992a
IE 9-11 cuts values 2 digits after the decimal point leading
to small miscalculations throughout the interface derived from `.mw-body-content`'s `em` value.
With `calc` it's forced to use same parent value for its
rendering calculations.
Bug: T102364
Change-Id: Id4ba39bc90174bab445ae0fb4d039c28a4f0b300
This commit is singularly focused on adding a link to the sidebar for
Vector, logged-in users. It does the bare minimum to fulfill the
requirements of T243281.
Additionally, it will help to answer the question "Do we need to use
abstractions (other than maybe different templates) to separate Legacy
Vector from Vector" by intentionally leaving out any abstractions in
order to make it easier to compare with a follow-up patch
(Ib2ef15180df73360cc1de25b893e49d415d23e1a) which does use abstractions.
It is a good thing to question whether or not we need addtional
abstractions in VectorTemplate and if they will help us as unnecessary
abstractions can have the opposite effect and just lead to further
frustrations down the road.
Therefore, I urge you, the reviewer, to let me know your thoughts! If
abstractions are viewed as not making our lives any easier, the
follow-up patches may be completely discarded and that's totally okay
with me. :) I think it's a good think to talk about now though.
Important changes:
* The VectorTemplate constructor was changed to allow injecting the
config, templateParser, and isLegacy boolean (only the config was
allowed before this commit). According to MediaWiki's Stable Interface
Policy, "Constructor signatures are generally considered unstable unless
explicitly declared stable for calling" [3]. Given that VecorTemplate's
constructor is not marked as stable, it is justified to do this without
warning according to the policy.
* Due to the above, the 'setTemplate' method is no longer needed and was
marked as deprecated.
* VectorTemplateTest was made to adapt to the new VectorTemplate
constructor. Additionally, it now extends from
MediaWikiIntegrationTestCase which my intelliphense server can pick up.
I *think* MediaWikiTestCase is just an alias to
MediaWikiIntegrationTestCase [1] and MediaWikiTestCase file was renamed
to MediaWikiIntegrationTestCase in [2], but I'm willing to change it
back if there is pushback to this.
Open questions:
* What are VectorTemplate's responsibilities? To me, it acts right now
as a controller (because it echos the full HTML string from the
template), a model (because SkinTemplate::prepareQuickTemplate sets data
on it which it later retrieves through `$this->get()`), a presenter
(because it adds data tailored for a web-centric view), and a view
(because it renders HTML strings instead of letting the view/template be
solely responsible for that). Arguably, some business logic might be
mixed in there as well (because it checks to see if a User is logged
in/has necessary permissions to show x which my changes here add to).
This might not be a problem if we keep VectorTemplate relatively small,
but will it remain this way as we progress further in Desktop
Improvements?
* How do we write tests for VectorTemplate without exposing unnecessary
public methods? For example, if I want to test the `getSkinData()`
method to see what state will be sent to the template, how should I do
this? One option might be to use `TestingAccessWrapper` to expose these
private methods which is what
`VectorTemplateTest::testbuildViewsProps()` does. Another option is to
accept this method as public. Is there a better way? Keep in mind that
even with access to this method, there might be many things to mock.
[1] 0030cb525b/tests/common/TestsAutoLoader.php (L64)
[2] Ie717b0ecf4fcfd089d46248f14853c80b7ef4a76
[3] https://www.mediawiki.org/wiki/Stable_interface_policy
Bug: T243281
Change-Id: I0571b041bcd7f19bec9f103fa7bccdd093f6394d
Address some feedback from I7a2cdc2dfdf20d78e4548f07cf53994563b234b3:
- Miscellaneous documentation improvements.
- Add a false case test to `DynamicConfigRequirement->isMet()`.
Bug: T244481
Change-Id: Ic5637f42da755f871c5a6d545e14effd3ac8c670
Various skins may extend SkinVector (although none are in production)
(https://github.com/search?q=%22extends+SkinVector%22&type=Code)
This makes it clear we no longer support that behaviour.
Because of the skins that are extending SkinVector currently we have
not resorted to using the final keyword at this time.
Depends-On: Ie3759c2acbf53c628577f6b05cfed17e0998a6bb
Bug: T248399
Change-Id: I2af8b2930c80f888791247bdaa2ae1c80576317e
Caused by If32b59036e5cd62cbb804944ca93fa1a101c5129. Looks like
that patch clashed with Ic7af947cfd5a5df4218f006232ede4ee7ed36c62
which touched same good without a merge conflict.
Bug: T247566
Change-Id: I0c918842ac2243e47bda702fb8fdc19ce57b005e
* Content.mustache doc param order was modified to follow the html structure
order (with html-dataAfterContent at bottom ). `html-userlangattributes`
was added to param documention since it was absent.
* Footer.mustache doc param order was modified to follow the html structure
order (with `html-userlangattributes` placed after `html-vector-before-footer`)
* Navigation.mustache was modified to remove `html-sidebar`,
`html-navigation-left-tabs` and `html-navigation-right-tabs` params from
doc since those are absent from Navigation.mustache.
* `data-namespace-tabs`, `data-variants`, `data-page-actions`,
`data-page-actions-more`, `data-search-box`, `data-sidebar` params were
added in Navigation.mustache documentation since those are present.
Bug: T243281
Change-Id: I321f8d61ad0305f508521e8d4d805e846103b357
We expect the vast majority of requirements and features to be defined
in services as possible. However, there are some "complex" requirements
that require additional application/HTTP request state. Unfortunately,
service wiring is done before some of that state is available.
I65702426 attempted to work around this by requiring clients of the
Feature Manager to pass that additional state on every interaction with
the system. Those complex requirements would then select the parts of
the state that they required when it was required. However
implementations of \IContextSource are God objects and their use should
be limited.
Whilst reviewing I65702426, Stephen Niedzielski mentioned that the
application state being available is a requirement. This remarkably
simple solution:
- Keeps the Requirement interface and FeatureManager API free of God
objects;
- Is true to the nature of the Feature Manager - it makes clear and
centralizes the various checks for application state being available
across the codebase; and
- Inject a Requirement implementations' dependencies at construction
time
It just so happens that the $wgFullyInitialised variable flags whether
the application state is available...
Changes:
- Add the the FeatureManager\Requirements\DynamicConfigRequirement class
and tests. The DynamicConfigRequirement lazily evaluates a single
configuration value whenever ::isMet is invoked
- Register an DynamicConfigRequirement instance, configured to evaluate
$wgFullyInitialised while constructing the Vector.FeatureManager
service
Bug: T244481
Change-Id: I7a2cdc2dfdf20d78e4548f07cf53994563b234b3
Lift the mists of confusion by checking that all JavaScript types align.
No ignores! This is the JavaScript equivalent to Phan.
This patch adds the necessary infrastructure for verifying typing and
fixes the few flaws found.
Bug: T239262
Change-Id: I2557471421196ea46cd13dfb786a52968fbfcc97
package-lock.json regenerated using the following process to ensure a clean slate:
rm -r node_modules package-lock.json ; npm i
Bug: T247004
Change-Id: Ie7720509ef37b178538a54854c28153ed73e375e
To have a clean break for upcoming changes we will duplicate
index.less into legacy.less and create a new module to clearly
separate new styles from old.
The preferred name however does come with some caching challenges.
Cached HTML served to anons will continue to load the style module
`skins.vector.styles` for a period of 1-4 weeks
Provided we are careful with our changes during this period this
should be okay.
Change-Id: If32b59036e5cd62cbb804944ca93fa1a101c5129
