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

tag: further improvements #487

Merged
merged 4 commits into from
Mar 30, 2019

Conversation

siddharthvp
Copy link
Member

  • simplified Twinkle.tag.callback.evaluate, reducing the lines of code as well as making to easier to maintain. Now if you add subgroups to some tags, you no longer have to update anything in this function.
  • file tags: convert tag-specific prompt dialogs to subgroups for better user experience

reduces the lines of code and makes it easier to maintain
modules/friendlytag.js Outdated Show resolved Hide resolved
subgroups for better user experience
@Amorymeltzer
Copy link
Collaborator

@siddharthvp This looks mostly fine from reading through the code, although I'll hold off on testing and doing a thorough review until the merge conflicts are dealt with. I'll probably be squashing them given things like the leftover console.log and the includes/indexOf.

@siddharthvp
Copy link
Member Author

until the merge conflicts are dealt with.

Done.

Copy link
Collaborator

@Amorymeltzer Amorymeltzer left a comment

Choose a reason for hiding this comment

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

Needs some changes to get working

modules/friendlytag.js Outdated Show resolved Hide resolved
modules/friendlytag.js Show resolved Hide resolved
modules/friendlytag.js Outdated Show resolved Hide resolved
modules/friendlytag.js Outdated Show resolved Hide resolved
modules/friendlytag.js Outdated Show resolved Hide resolved
@Amorymeltzer
Copy link
Collaborator

Amorymeltzer commented Mar 29, 2019

@siddharthvp In addition to the fixed required fixes in the above review, the changes to file tagging simply don't work. I didn't look into why, but they all end up as undefined and a number of the parameters are repeated. Compare the changes made with the current tag module to those made with this version

modules/friendlytag.js Show resolved Hide resolved
@Amorymeltzer
Copy link
Collaborator

It's certainly different, but I really like the changes here! Smart and nifty, cleans things up real well, and should make it much easier to maintain 🏆

@Amorymeltzer Amorymeltzer merged commit e54abb3 into wikimedia-gadgets:master Mar 30, 2019
Amorymeltzer added a commit to Amorymeltzer/twinkle that referenced this pull request May 8, 2019
Closes wikimedia-gadgets#624.  {{R from alternative name}} has a div element, and the processing schema introduced in wikimedia-gadgets#487 dies on it
@siddharthvp siddharthvp deleted the tagsimplify branch October 22, 2020 20:23
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.

None yet

3 participants