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

twinkle.js: group all portlet code in a class #1836

Closed

Conversation

NovemLinguae
Copy link
Member

@NovemLinguae NovemLinguae commented Jul 26, 2023

Moved away from storing skin variables in the preferences system, which is confusing, makes them seem like they're configurable at Twinkle/Preferences, but they are not.

Also renamed a variable, and deleted some variables that weren't used.

Later, will refactor that class a bit more so that we can add some unit tests. For example, document should probably be passed in through the constructor so it can be mocked.

What is $.collapsibleTabs? Can that be deleted? Doesn't look like it's ever set.

@siddharthvp
Copy link
Member

Moved away from storing skin variables in the preferences system, which is confusing, makes them seem like they're configurable at Twinkle/Preferences, but they are not.

They are configurable by editing twinkleoptions.js directly (which is why they are in the Hidden section), but yeah almost any change made usually causes the menus to become weird as we don't actively support customisability. In effect, no one uses the feature. So agree with pulling them out of config.

What is $.collapsibleTabs? Can that be deleted? Doesn't look like it's ever set.

It's defined in vector 2010 skin, comes from skins.vector.legacy.js module.

@NovemLinguae
Copy link
Member Author

My searches indicate that 6 users are using the hidden skin preferences, one of which is probably a developer's test account. Not ideal to disrupt user's preferences, but I think it's acceptable since this patch will enable further refactoring that will result in the portlet code being much more maintainable.

@NovemLinguae
Copy link
Member Author

This patch is kind of big, and I have some reservations about introducing ES6 classes when the rest of the code is still on ES5 style classes. I think I'll abandon this patch and then submit a smaller patch that just eliminates portlet preferences.

NovemLinguae added a commit to NovemLinguae/twinkle that referenced this pull request Jun 22, 2024
- No longer allow users to set portlet preferences. These hidden preferences were rarely used (6 instances found), and increase technical debt by making the code complex. I think it makes sense to eliminate them.
- Move defaults for these values to the Twinkle.addPortlet() function, a more natural spot for these.
- One of these preferences was used outside the Twinkle.addPortlet() function, so modify Twinkle.addPortlet() to return the preference and place it in a variable.
- Add a comment about what $.collapsibleTabs is.

Related wikimedia-gadgets#1836

Note to self: not manually tested yet.
@siddharthvp
Copy link
Member

... I have some reservations about introducing ES6 classes when the rest of the code is still on ES5 style classes.

I think that's fine, as we have to start somewhere. Can't refactor the whole codebase to ES6 in one go.

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

Successfully merging this pull request may close these issues.

2 participants