Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix "more" menu's styling in skin vector-2022 #1693

Conversation

NovemLinguae
Copy link
Member

@NovemLinguae NovemLinguae commented Jan 5, 2023

Co-authored-by: @MusikAnimal

Bug report: https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Styling_for_the_TW_menu_gone%3F

MusikAnimal's hotfix: https://en.wikipedia.org/w/index.php?diff=1131811156

I also refactored this because I found it unreadable. The hotfix (included in this patch) is adding a "vector-dropdown" class when appropriate. The rest of the code changes (the refactoring) should be a no-op.

@github-actions github-actions bot added the Module: twinkle The twinkle.js global gadget file label Jan 5, 2023
Copy link
Collaborator

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also refactored this because I found it unreadable.

Yes, nested ternary operations should be forbidden by the linting tests! I too was confused and that's what deterred me from making a PR myself, hehe :)

Code LGTM, though I haven't tested. Longer-term I have concerns that the "page tools" stuff (T302073) will break Twinkle again. No action needed for now seeing as we don't know what exactly will change, but maybe, just maybe they'll let us know before the changes are deployed...

if (navigation === 'mw-panel') {
outerNavClass += ' vector-menu-portal';
} else if (type === 'menu') {
outerNavClass += ' vector-menu-dropdown vector-dropdown vector-menu-dropdown-noicon';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vector-menu-dropdown-noicon doesn't seem to have any effect anymore. I ended up removing it from MoreMenu. It's still being added server-side by legacy Vector, and it's even being checked in acceptance tests, but it doesn't appear to be used anywhere else. I asked about it at T291438#8503744. For now I guess let's keep it.

@MusikAnimal MusikAnimal merged commit 49207b0 into wikimedia-gadgets:master Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: twinkle The twinkle.js global gadget file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants