Page MenuHomePhabricator

Merge the 'collapsiblenav' component of Vector extension into the Vector skin
Closed, ResolvedPublic

Description

(Split from bug 45051.)

Considered stable, enabled on WMF wikis by default, currently configurable
in preferences. I don't like it personally and I have it disabled in my
prefs; it should probably be merged, but I'd like to keep the possibility of
disabling it in prefs.


Version: 1.19
Severity: normal

Details

Reference
bz46512

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 1:15 AM
bzimport set Reference to bz46512.

I volunteer to take care of this.

After some idle weeks I return to work, sorry for the delay.
I'm new to Git and Gerrit, I've been looking carefully at this issue but I'm not sure how to proceed.
The collapsibleNav feature of the Vector extension has elements spread all over the extension files, and I'm not sure how to bring them to the Vector skin without generating conflicts later on with the merging of the other features. For example, the option to disable the collapsibleNav feature is triggered by a hook, which is a method of the VectorHooks class. This method uses a property of the class which contains references to other features of the Vector extension, such as the footerCleanup feature. Should I just remove everything not related to collapsibleNav from the VectorHooks class, and push the result? If I do this, then when merging the footerCleanup changes, a conflict may arise.
Should I push the VectorHooks class more or less as it is, even if it contains references to other features?
What is the right way to proceed in these cases?
Any help is appreciated, thanks.

(In reply to comment #0)

Considered stable, enabled on WMF wikis by default, currently configurable
in preferences. I don't like it personally and I have it disabled in my
prefs; it should probably be merged, but I'd like to keep the possibility of
disabling it in prefs.

I don't like the user preference and think it should be killed. On the other hand, I don't use Vector.

(In reply to comment #2)
Bartosz: any chance you could reply to comment 2?

After some though, I wouldn't mind killing the pref entirely. (I actually reenabled it for myself some time ago.)

(In reply to comment #3)

(In reply to comment #2)
Bartosz: any chance you could reply to comment 2?

I think the best way to answer this is, instead of poiting out each caveat, to point to fixes to bug 46513 and bug 46514, as this is exactly the same thing as there.

I'll look into this. It will help with some changes I am currently work on in Vector.

Change 83590 had a related patch set uploaded by Jdlrobson:
Move collapsibleNav to core

https://gerrit.wikimedia.org/r/83590

Stats for enwiki and dewiki:

mysql:wikiadmin@db60 [enwiki]> select * from user_properties WHERE up_property='vector-collapsiblenav';
......
1584 rows in set (0.99 sec)

mysql:wikiadmin@db60 [enwiki]> select max(user_id) from user;
+--------------+

max(user_id)

+--------------+

19746121

+--------------+
1 row in set (0.00 sec)

0.008%

mysql:wikiadmin@db74 [dewiki]> select * from user_properties WHERE up_property='vector-collapsiblenav';
......
380 rows in set (0.28 sec)

mysql:wikiadmin@db74 [dewiki]> select max(user_id) from user;
+--------------+

max(user_id)

+--------------+

1733493

+--------------+
1 row in set (0.00 sec)

0.02%

Looks pretty low usage to me...

Change 83591 had a related patch set uploaded by Krinkle:
Move code for navigation collapsing to core

https://gerrit.wikimedia.org/r/83591

Note that those 1584 enwiki accounts have both value='' and value=1. They aren' all opt-in or opt-out.

Did another query limiting to where up_value!=1 (as it is enabled by default) and LEFT(user_touched, 8) >= '20130301' (filter out users who have no activity in the last 6 months).

Also ran it on an RTL wiki and multi-language wikis as they may be other trends there.

SELECT user_touched, up_property from user, user_properties WHERE up_user=user.user_id AND up_property='vector-collapsiblenav' AND up_value!=1 AND LEFT(user_touched, 8) >= '20130301';
...

English, German, Dutch

[enwiki] 902 rows in set (0.03 sec)
[dewiki] 318 rows in set (0.39 sec)
[nlwiki] 58 rows in set (0.36 sec)

Hebrew

[hewiki] 19 rows in set (0.05 sec)

Multi-language

[commonswiki] 159 rows in set (0.64 sec)
[metawiki] 67 rows in set (0.34 sec)

Change 83590 merged by jenkins-bot:
Remove collapsibleNav module

https://gerrit.wikimedia.org/r/83590

Change 83591 merged by jenkins-bot:
Vector: Add navigation collapsing feature

https://gerrit.wikimedia.org/r/83591

Excerpt from bug 55905, comment 10:

  1. The new implementation of the collapsiblenav seems to be imperfect: On

initial page load the toolbox is not collapsible and all items are expanded.
After some delay the collapsiblenav content seems to be loaded (e.g. arrows
appear, indentation changes) and some of the items collapse.
It should be collapsible from the start or not collapsible at all - not
something in between that gets slowly loaded with notable delay after page
load.

I think the implementation should be optimized especially after the option to disable the collapsing was killed.