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

Make MoreMenu gadget more future proof #26

Closed
wants to merge 1 commit into from

Conversation

jdlrobson
Copy link
Contributor

@jdlrobson jdlrobson commented Mar 15, 2022

Clone an existing menu to work out the menu structure. (Follow up to #25#issuecomment-1064558959 )

@MusikAnimal what do you think about this patch? While it won't prevent breakage, I think it will reduce the breakage. It will also throw an error when action needs to be taken, so when it does break we'll know right away (hopefully via an email alert).

@jdlrobson
Copy link
Contributor Author

(Full disclosure: final code untested as I couldn't work out how to get it running locally)

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.

Of course, the issue is not just with MoreMenu, but until we have official mw.addPortlet support this is better than nothing. Thank you so much for taking the time to write this! I will test it soon.

Full disclosure: final code untested as I couldn't work out how to get it running locally

I have updated the README with instructions. It's pretty easy to set up, and you can even test on a local MW install if you want.

dist/MoreMenu.js Outdated
} else {
// Throwing an error will notify Wikimedia developers that they
// broke this script by showing up in our error logs.
throw new Error('MoreMenu: p-cactions is not in DOM. Expection with Vector maintaining team not met.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clever! So this will show in logstash?

Also, typo: expection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

This is a good idea for any popular and maintained script as it will show up in https://grafana.wikimedia.org/d/000000566/overview?orgId=1&viewPanel=16 which will block the train if we go into red zone. It should also show up on the beta cluster if you have a script to update it there, which gives a small chance that any issue will be picked up before the train begins to roll out.

It's also a nice way to setup an informal but enforceable contract with us :)

src/MoreMenu.js Outdated Show resolved Hide resolved
@jdlrobson
Copy link
Contributor Author

Thanks for updating README. Run out of time today, but can take a look tomorrow and give the readme a spin.

@jdlrobson
Copy link
Contributor Author

Amended and tested:
Screen Shot 2022-03-17 at 1 19 37 PM

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.

Works beautifully! Just one minor organization issue with the code. If you're busy I'm happy to merge now and fix later :) Thanks for taking the time to write this PR!

src/MoreMenu.js Outdated Show resolved Hide resolved
Clone an existing menu to work out the menu structure.
// broke this script by showing up in our error logs.


throw new Error('MoreMenu: p-cactions is not in DOM. Expection with Vector maintaining team not met.');
Copy link
Member

Choose a reason for hiding this comment

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

There's a typo at "Expection". Also I can't figure out the meaning of the second sentence. Consider re-wording.

}
// Throwing an error will notify Wikimedia developers that they
// broke this script by showing up in our error logs.
throw new Error('MoreMenu: p-cactions is not in DOM. Expection with Vector maintaining team not met.');
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@jdlrobson
Copy link
Contributor Author

I had another idea of how we might approach this here - https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/878212 I quite like this idea as it allows skins to reimplement the mediawiki.util methods. If not implemented in the skin it would throw an exception so the more menu code would look something like

try {
  var moreMenu = mw.util.addPortlet( 'p-more', [ { href:'#', text: 'link 1' } ] )
} catch (e) {
 // existing code.
}

@jdlrobson
Copy link
Contributor Author

@MusikAnimal looks like this broke again: https://phabricator.wikimedia.org/T337893

I'm still out on vacation, but perhaps we could work together on the core code to find a more long term solution here.

@MusikAnimal
Copy link
Collaborator

Almost 2 years since this PR was opened! I think now that we're using mw.addPortlet, the risk of future breakage is a bit lower. Changes to CSS classes on individual menu items is still prone to breakage, but I'm comfortable with updating MoreMenu as necessary when this happens, so I will close this PR.

I want to thank you @jdlrobson for going out of your way to create this PR, and for all of your efforts to mitigate conflicts with scripts/gadgets as Vector 2022 evolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants