-
Notifications
You must be signed in to change notification settings - Fork 144
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
Switch to {{socklist}} in SPI report #1509
Conversation
Makes it easier for people to add more sox after the report is filed, and allows for a working set of tools links. Templates/modules do all of the heavy lifting. See https://en.wikipedia.org/wiki/Template_talk:SPI_report#Proposed_change_to_sock_list.
@Dreamy-Jazz FYI |
Looks good to me. I would note that consensus about this template is likely not to take too long, so this pull request may be ready to go soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified this change works using Firefox inspect element's console. No changes needed to the code, and as long as this doesn't have major opposition on-wiki I think this would be fine to merge.
Didn't work on testwiki. May be an out-of-date or missing template on testwiki, or may be a bug in the code. Can you investigate? |
@NovemLinguae Yeah, there was a lot that had to be imported first. Think I've got it sorted. Try again? |
Works now, thanks for updating those templates. The side by side looks good, the old and new code render exactly the same as far as I can tell. Detailed discussion onwiki, for the record: https://en.wikipedia.org/wiki/Template_talk:SPI_report#Proposed_change_to_sock_list I'll go ahead and make a post on the above wiki page, and pause for responses, and also to give @siddharthvp a chance to weigh in. If everything looks good in a couple of days, I'll merge it in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I've proposed this as the new default for {{SPI report}}, and think Twinkle should use it as well. It would eliminate the longstanding problem that Twinkle reports, because they use
{{{socksraw}}}
, do not generate editor interaction links. This way, {{SPI report}} need only pass usernames as numbered parameters, and {{sock list}} does all the heavy lifting. Also makes it easier for people to add more sox after the report is filed. See https://en.wikipedia.org/wiki/Template_talk:SPI_report#Proposed_change_to_sock_list.Resolves #1505 by superseding, just because it coincidentally affects the same lines of code. I can re-add the two bytes that 1505 removes if y'all would prefer they stay two separate PRs.
Blocked until the relevant change is implemented to {{SPI report}} (pending consensus).
Obligatory disclaimer that I am not very good at JavaScript, and think this change should work but am not 100% sure.