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 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
* 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
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
This allows us to insert HTML underneath the first portal or wrap
the portal in a containing element in future if we want to target
additional CSS to it
Change-Id: Ied28d95407b8d59fc819bb07a2cce3242bd93088
FeatureManager::registerRequirement established the interface for a
requirement: its name and whether it's met.
However, the Feature Manager also needs to handle scenarios where a
requirement needs additional context before it can be considered met.
That context may not be available when the application is booting, e.g.
checking if the user is logged in; or the logic is complicated enough
that it should be under test.
Changes:
- Add the Requirement interface and update FeatureManager to work with
implementations of it
- Maintain B/C by constructing an instance of a the SimpleRequirement
DTO
Bug: T244481
Change-Id: Id95d9e5d7125492968d0e15515224aadbc3075f8
I735fd640 bumped the required MediaWiki version to 1.31. That version
dropped support for PHP 5.x.
Wherever possible, update FeatureManager's methods to use PHP 7.0.x's
scalar and return type declarations.
Bug: T244481
Change-Id: Ib5636d0ec5ec7f0c93b5b3317a12635668b589e2
Separate value type from value groups, to clarify that e.g.
something coming from a hook does not mean it should not have "html-".
Change-Id: I52fdd6995241e1d64f23fd8154997de813f674f4
As was noted in https://phabricator.wikimedia.org/T244481#5859513, the
term "set" doesn't seem natural. Piotr Miazga (polishdeveloper, pmiazga)
and Nicholas Ray (nray) suggested a number of good replacements,
including "requirement." Serendipitously, this term is already used in
FeatureManager's documentation.
Bug: T244481
Change-Id: I559c2d4149db69235cdd4bb880697deb1a145743
CHANGES to index.mustache:
Changes can be understood by looking at the diff of
the file stories/skin.stories.js
The additional changes in stories folder export the data passed to
those templates. A new file is used as exporting any variable in a
file suffixed stories.js will be assumed to be a story entry.
The changes to index.mustache are as follows:
* html-footer is replaced with data-footer and Footer
component is rendered via template partial
* html-navigation is replaced with data-navigation and
Navigation component is rendered via template partial
CHANGES to Navigation.mustache:
Changes are best explained by looking at the diff to
stories/navigation.stories.js
and navigation.stories.data.js
* html-personal-menu is replaced with data-personal-menu
Bug: T245456
Change-Id: Ie96e92447a932b8a7f3844df277a1d31a2af423c
Sections can be nested. Using rendering/skin as the parent
means that Vector's skin preference will always come straight
after the skin preference and before Popups.
A change in core is needed to update the selector for the element which
shows/hides the subsection as well as provide a generic message key to
replace the one inside this repository.
Note: If the "Vector" specific heading is needed, we can achieve this
with a little more work but that is a conversation for another time.
Depends-On: Idd06bcfe7935e16732a6a95c1253dbf95c8aca2e
Bug: T246162
Change-Id: I4be9764ddca186e5bfd493678afd62d446072e8f
For better encapsulation and future possibility to create Skin API
we have to stop accesing properties and start using getters/setters
instead. Once we start doing that we should be able to provide a
clear interface for our templating system.
Bug: T246161
Change-Id: Ib3539b1e3bc12341c79913af3c95acad8619cff4
Moving all the templateParser calls to one function
so its easier to see how the template is composed.
The diff of changes to the stories folder highlight
the internal changes which are:
* html-portals replaced with html-sidebar in main template
* new Sidebar template added which outputs to html-sidebar
* Mention of "MainMenu" replaced with better understood "Sidebar"
This is precursory work to adopt templatePartials
Change-Id: I6b2196e39087f818e774d04b2d1b9ab8cb8816a1
Add a Vector-specific user preference to Special:Preferences for
toggling skin version, either Legacy Vector or the latest Vector.
The presentation of the new preference section and the default values
for anonymous, new, and existing accounts are configurable via
$wgVectorShowSkinPreferences, $wgVectorDefaultSkinVersion (to be used by
the feature manager in T244481),
$wgVectorDefaultSkinVersionForExistingAccounts, and
$wgVectorDefaultSkinVersionForNewAccounts. These configurations default
to the fullest experience so that third-party configuration is minimal.
See skin.json for details. The configurations are each tested in
VectorHooksTest.php.
When presentation is enabled, the new preference appears as a checkbox;
enabled is Legacy mode and disable is latest. There are a number of
unfortunate details:
- Showing and hiding a checkbox is supported by OOUI. Showing and hiding
a whole section (Vector skin preferences, in this case) is not so this
additional client JavaScript functionality is added in Core (see
Iaf68b238a8ac7a4fb22b9ef5d6c5a3394ee2e377).
- Stylization as a checkbox is wanted. However, the implied storage type
for OOUI checkboxes is a boolean. This is not wanted in the event that
another skin version is added (e.g., '3' or 'alpha'). As a workaround,
the preference is converted from a boolean to a version string ('1' or
'2') on save in Hooks::onPreferencesFormPreSave() and from a version
string to a checkbox enable / disable string ('1' or '0') in
onGetPreferences(). There a number of test cases to help cover these
concerning details.
Documentation for overriding the skin version as a URL query parameter
is provided in anticipation of T244481.
Bug: T242381
Bug: T245793
Depends-On: Iaf68b238a8ac7a4fb22b9ef5d6c5a3394ee2e377
Depends-On: Ifc2863fca9cd9efd11ac30c780420e8d89e8cb22
Change-Id: I177dad88fc982170641059b6a4f53fbb38eefad6
With complex additions to Vector's codebase like the Desktop Improvement
Program upcoming, it's important that we have a shared, intuitive
language to talk about features and their requirements. Centralising
the registration of features and creating an API satisfies does exactly
this.
This change introduces a greatly-reduced version of Piotr Miazga's
(polishdeveloper, pmiazga) original proposed API and associated
scaffolding classes for feature management in Vector, which itself was
based upon his work in MobileFrontend/MinervaNeue. This is done to
establish a foundation upon which we can build the more sophisticated
parts of Piotr's proposal in a piecemeal basis, thereby minimising risk.
Distinct from Piotr's proposed API is the ability to register sets and
features that are always enabled or disabled.
Additionally:
- A Vector.FeatureManager service is registered but not used
- A list of proposed immediate next steps is included
Bug: T244481
Change-Id: Ie53c41d479eaf15559d5bb00f269774760360bde
The renderNavigation function provides an unnecessary level of
abstraction which I'd argue hurts readability of these functions.
Let's remove it.
Change-Id: Iad1c4db606404fecf4d5ae98981df9f05d3f661e
Thanks to the dependent change, the print logo is now provided
in core so we can remove the custom Vector ResourceLoader module
ResourceLoaderLessModule is replaced with a ResourceLoaderSkinModule
and gains the features capability replacing the need for
'mediawiki.skinning.interface' making use of the changes added in
6845912bcf1.
Note that for cached HTML both 'mediawiki.skinning.interface'
and skins.vector.styles will be loaded. We can avoid this
by renaming skins.vector.styles if necessary (but I'm not
sure if we'd want to do that)
Bug: T232140
Depends-On: I00899c16c0325f36b671baf17e88c2b5187b3526
Change-Id: I569e0d800e147eabc7852567acd140108613f074
- Update package.json with the new dependencies.
- A script storybook.sh pulls down CSS and LESS imports from external
dependencies. This copies the approach taken in Popups and MobileFrontend.
- Icons from external repos are maintained within the repo in SVG-only form.
Using load.php modules is also possible, but will pull down other unnecessary icons
and break if any of these modules are changed. Decided that we should manually maintain
these for the time being given there are only 3 icons.
- Several LESS files now import the variables file. I think it's useful for stories
to only import the CSS they use as this encourages us to modularise our CSS. Before these
imports were not necessary as they inherit imports from index.less. This will have no impact
on the bundle size as the LESS compiler silently discards duplicate imports
- stories/utils.js provides a useful placeholder function for generalising our hook entry
points.
Bug: T242674
Change-Id: I722e84d2fb57653a2f96142dc3e5248043261746
Use a template variable only for html attributes of the logo
Move static HTML into Navigation.mustache
Change-Id: If37015b9ce4f37e264b6f25956e4d0ca35e8cdff
Vector provides two vector specific hooks:
- VectorAfterToolbox
- VectorBeforeToolbox
Instead of supporting Vector specific stuff we should aim
for a nice global skin api and allow other 3rd party to
extend MediaWiki functionality no matter which skin is currently
on.
Bug: T240062
Change-Id: I245f1316e79f814ba04f4e0a0223d4f0596cf39e
This will allow us to render in Storybook without having issues
with unclosed tags. it also mirrors how html-headelement works
(obscuring the opening of the body and html tags)
Bug: T240062
Bug: T242674
Change-Id: I216a920c68bf3da9de55a75fc53451c68c9cc753
A Portal (or portlet) in the Vector context is a section of the
sidebar menu (e.g. Tools, Languges etc.).
The hook that places content between the portals (or portlets?)
such as `SkinTemplateToolboxEnd` and `otherlanguages` has been
enclosed inside of an output buffer so that any content it
produces can be passed to a template.
Bug: T239248, T240062
Change-Id: I882db161e5462cf88aa48c9cfd91448eb97a4a77
Extracts a new VectorMenu mustache component from VectorTemplate.
VectorMenu is the "more" menu that appears at small widths instead of the
Read/Edit/View History menu near the top of the Vector skin.
Bug: T239248, T240062
Change-Id: I41b1ec949d81303abddadb981741445572c939e3
The SkinVector implements IContextSource, thus it has to provide
the getConfig() method. Vector skin is currently using
it's own `vectorConfig` in one place and it passes the VectorConfig
to the VectorTemplate class. Sadly the VectorConfig is the same
thing what $this->getConfig() provides. There is no need to make
this code more complex by handling two different config containers
which at the end are the same GlobalVarConfig instances.
If we decide to handle VectorConfigs differently, let's thing it
through, for now we should simplify code and remove all uncessary
logic. Thanks to that, there is also no need to override the
setupTemplate().
Change-Id: I89c8a77f7d96f867c8c72e61f9e104e14d9512d9
Tthe SkinVector::steupSkinUserCss is deprecated and
we should use the ::getDefaultModules instead.
Change-Id: I359049d0f159a0dfc374d1c0124e8989aeb4182d
PersonalMenu is the login/logout notifications etc. menu at the very
top of the Vector skin.
Bug: T239248, T240062
Change-Id: Iae224cbd838e44669a9f27e6dd303c6c3b402d41
For consistency with VectorTabs, rename SearchComponent to only imply
component. At least two word names seems like a good target (instead of
just "Search") for grepability and standard component style conventions.
Bug: T239248
Change-Id: I1e4f7270ba29c2f35f08e92f8a28cd8a2ec8fe87
The tab attributes were mistakenly being echoed instead of passed by
value. E.g., http://localhost:8181/wiki/A?uselang=he.
Bug: T239248
Change-Id: Ia0af96df717948743a2d66f8432b47e6f8a9c8f1
Creates a new VectorTabs.mustache component and uses it to render
the namespace & view tabs.
Bug: T239248
Change-Id: I859e4e95a2a12470f66564db547679c9f0a16727
Extract SearchComponent.mustache from VectorTemplate. The "search"
message is now escaped by the template parser. As is, htmlspecialchars()
for "wgScript".
A later patch will change each component renderer function as well as
renderNavigation() to return a string.
Change-Id: I3084b7e0ef73d320c85ee780c9eff13ecea92906
Move template logic outside of the template and into variables. This
will allow a future refactor to move the template to a separate
template-only file.
Change-Id: I91c2df11106cf09428a890a2d96ecbddb18ef0fe
Cut up renderNavigation() into several discrete methods. Smaller methods
are easier to read and reason about.
Change-Id: I4e55d858ab1745fb2baddef02f7046ec842eb149
Looks like the SkinVector::getPageClasses is doing nothing, only
calling the Skin::getPageClasses and returning the output.
Change-Id: I5becaa1505d46cbf9a07b8bfe73ddf324b740876
With this change the constructor of the parent class gets called like if
there is no local constructor. This allows the ancestor classes to
initialize themself in their constructor.
Change-Id: I4ec1ec935afa25b59be4020fca9ac71f23e62ad0
Regression from 74b9803d9a, caused by a bug in LightNCandy which
caused {{foo}} to render "0", and {{#foo}} to pass as true with "0", but then
in {{#foo}}<b>{{foo}}</b>{{/foo} render as empty string producing "<b></b>".
In other words, the conditional is passing and the inner block is executed,
but the placeholder is mistakenly converting "0" => null => "" (empty string),
causing the <h1> to render but without any text in it.
Work around this bug by simply removing the conditional. Several other skins
already don't have this conditional and it's unclear why or in what
situation MediaWiki would send OutputPage to SkinTemplate without a title.
I think it would make sense in such rare case to still have a consistent
layout for extensions and gadgets to interact with and not omit the H1
element, but render it with the value that OutputPage gave it, even if it
is the empty string.
Bug: T219864
Change-Id: I6e04b512d2fe2e949ff5385cb38ceebe392fb255
This reverts commit a3ca2c3e16.
Reason for revert: This requires wider discussion before moving
forward, and a more complete implementation even once we do have
consensus.
No associated task exists on which to view or continue this
discussion: linked task briefly mentions Mustache in general as an
option as part of a much wider topic, but doesn't concern this
specifically.
Issues that should be discussed include:
* What the intent even is here: is this for one skin only? Is this
the intended path forward for all of them? Depending on which, we
have other issues: for the former case, that it is quite
unhelpful in terms of maintenance and further development having
more random code diversity out there, especially in this
half-completed state; or if it is indeed intended for all of them,
that an RfC is needed before anything is merged, as that is a very
significant change.
* That using Mustache in MediaWiki does add (usually minor)
performance overhead; we need to clearly establish in the task that
this is indeed worth it here.
Change-Id: I0bafa55b554aa8a38553e20c75859ec5eec2c062
The VectorTemplate class extends BaseTemplate, which has defined
this method since MW 1.25. The Vector skin master branch supports
MW 1.29+ only.
Change-Id: I83c6add9e8c02df028ca5905934e7d367dbe2209
Xml::expandAttributes() outputs a space at the beginning before it
outputs the attributs.
This change avoids a double space between the attributes.
Before this change the HTML contains:
<a class="mw-wiki-logo" href="..." title="...">
After this change the HTML contains:
<a class="mw-wiki-logo" href="..." title="...">
Change-Id: I486d26bd56a4410766f40b78466c2f3559f3a1ff
Before this change the HTML contains:
<div id="p-personal" role="navigation" class="" ...>
or
<div id="p-personal" role="navigation" class=" emptyPortlet" ...>
After this change the HTML contains:
<div id="p-personal" role="navigation" ...>
or
<div id="p-personal" role="navigation" class="emptyPortlet" ...>
Change-Id: Ic686b958940afc958693d0031ac31e5f783960a9
Unordered lists can be absolute positioned down to IE 7.
Outdated selector `.vectorMenu ul` remains for a release cycle
until HTML cache is renewed.
Bug: T209558
Change-Id: Id18ca9a8d705572b1f7e17920ef52b80e9aec373
* Improve their accessibility by giving both links
a full label "Jump to x" and "Jump to y" instead
of "Jump to: ", "x", "y".
This also makes things much better for localisation, for which
we generally discourage use of concatenation.
* Use pure CSS for the toggling of the visibility on focus,
instead of relying on JavaScript. Especially given the
JS comes form core's 'jquery.mw-jump' module, which is
considered technical debt per T195256. Alternatively,
that could be copied to vector.js, but pure CSS
is possible, so why not.
* Use plain <a> links in the HTML instead of wrapped in a <div>.
This solves the long-standing problem whereby the margin
between #contentSub and #mw-content-text had to be awkwardly
negated and overridden in core and on various to make sure that
the wrapper itself would become visible as needed, in a way that
has margin around this. This whole problem doesn't apply when
simply using inline links that aren't part of the regular flow
with .mixin-screen-reader-text. On focus, the individually
focussed link appears in regular flow, without the need for
any custom styles.
* This uses :not(:focus) to naturally make it render in the default
way on focus, and visibibly hidden/clipped otherwise.
This is supported in IE9+ and Android 2+.
There is a way to make it work with CSS2 for IE7-8, by applying
the mixin to '.mw-jump-link' only and then undoing all of
'position', 'width', 'height', 'clip', and 'margin' on :focus.
But I'm not sure that's worth it here. The fallback in IE7-8
for not supporting ":not(:focus)" is that the accessibility
link is simply visible always, which seems like a good fallback
for accessibility, and doesn't hurt anything.
Bug: T195256
Change-Id: Icaadb290f692b3617688d32cbb66dfb007f1c82c
