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
Move styles.less to index.less and import print.less from it. This keeps
the reasoning about styles constrained to LESS instead of spread out
over LESS _and_ ResourceLoader. The former is preferable since LESS is
more standardized than ResourceLoader.
The approach of moving styles.less to index.less and then referencing
print.less was chosen with the intent that it'd be easier to assume
styles are screen styles unless a media query says otherwise.
This patch also makes the variables import common among print and screen
styles.
Bug: T246419
Change-Id: I981d0937aaacb7cba082c337f98c90e90b46b340
* Following up on the work from
Idf90ee2a0f1c1d08a31cf50099c0bebc7b67e619, this commit renames the
storybook files/storybook names to their respective component name.
e.g. personalNavigation.stories.js => PersonalMenu.stories.js
Bug: T243281
Change-Id: I68054663c5a597f90a826b6f75bf399382dca609
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
- 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
jdlrobson
Nicholas Ray
Stephen Niedzielski