Hi, @bwang and @Jdlrobson I'm a bit confused with the issue that what should be deleted from includes/SkinVector.php and what, how it should be handled in includes/templates/Menu.mustache file. If possible then please explain the issue in more detail as I'm not getting it.

Hi @abhigya_pandey
Yes the idea here would be to hardcode the classes in includes/templates/Menu.mustache rather than adding template data in SkinVector.
e.g. t[[ https://github.com/wikimedia/Vector/blob/master/includes/SkinVector.php#L892 | his line would be removed ]] and this line updated
Does that make sense?

Thanks Jdlrobson for nice explanation, now I got it.

Change 773445 had a related patch set uploaded (by Abhigya Pandey; author: Abhigya Pandey):

[mediawiki/skins/Vector@master] Move Vector menu class logic into Menu mustache template

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

We have a patch from volunteer, so adding to sprint board code review column

Thanks @Jdlrobson for supporting me to solve this issue, actually I had updated a line as-

but stuck with the statement-"You will also want to include some cleanup in this patch by removing these classes in other areas, I'm seeing it in test files and storybook."

I'm not able to get what changes have to be made in SkinVectorTest.php here-

and what have to be deleted in setup-storybook.sh here-

Please suggest me the required changes.
Thank You!

Hey @abhigya_pandey

Your change here https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/773445/2/includes/SkinVector.php is the right one, but you need to update the tests to reflect the new state.

You can see the storybook by running npm start
It's also published here:
https://doc.wikimedia.org/mediawiki-skins-Vector/master/js/ui/?path=/story/userlinks--logged-in-user-links

These stories are built from the code inside stories/UserLinks.stories.data.js. There is one which passes 'heading-class': 'vector-menu-heading', - with your change this needs to be updated to empty string.

See also stories/LanguageButton.stories.data.js

There is also a reference to vector-menu-heading in tests/phpunit/integration/SkinVectorTest.php that needs updating.

You can execute the tests using:

php ../../tests/phpunit/phpunit.php tests/phpunit/integration/SkinVectorTest.php``

I have made the necessary changes in patchset 3 but I was not able to find any required changes needed to be done by 'vector-menu' as I have done with 'vector-menu-heading' in all such files. I hope the changes with respect to 'vector-menu' has already been done or if not then kindly point out where the necessary changes have to be done.

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/0ae44b6779/w/

@abhigya_pandey patchset 3 still has some flagged issues. Let's focus on getting that patch finished first. Hopefully my new comments make it clearer what needs to change.

I have uploaded the required changes, while inspecting with the done changes we can see-

but 'Personal tools' is not visible here-

Change 773445 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Move Vector menu class logic into Menu mustache template

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

@abhigya_pandey so the remaining work here to resolve this ticket is to cleanup our users of vector-menu. Are you interested in doing this, or would you prefer to look at another task?

If you are happy to help us, the vector-menu class is set in SkinVector::decoratePortletClass however we'd prefer this to be set inside the template. Note, you'll want to be testing this on your localhost rather than storybook to avoid errors here.

Thank you @Jdlrobson, yes I'm interested in solving this issue completely and will look to your suggestions.

Change 788350 had a related patch set uploaded (by Abhigya Pandey; author: Abhigya Pandey):

[mediawiki/skins/Vector@master] Move Vector menu class logic into Menu mustache template

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

Change 788350 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Move Vector menu class logic into Menu mustache template

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

Thanks for wrapping this one up! Anything left to do here?

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/0ae44b6779/w/