Page MenuHomePhabricator

Add class to diff and history links in changes lists for easier selecting in CSS/javascript
Closed, ResolvedPublic

Description

For example in https://gerrit.wikimedia.org/r/#/c/333497/ the javascript has to navigate through DOM and make lots of strange assumptions to add simple highlighting.

Event Timeline

Ladsgroup created this task.Feb 4 2017, 3:29 AM
Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptFeb 4 2017, 3:29 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 335958 had a related patch set uploaded (by Ladsgroup):
Add class in diff and history links in ChangesList

https://gerrit.wikimedia.org/r/335958

Change 335963 had a related patch set uploaded (by Ladsgroup):
Add class to diff and history links in Special:Contributions

https://gerrit.wikimedia.org/r/335963

Ladsgroup updated the task description. (Show Details)Feb 4 2017, 3:31 AM

Change 335963 merged by jenkins-bot:
Add class to diff and history links in Special:Contributions

https://gerrit.wikimedia.org/r/335963

Ladsgroup moved this task from Incoming to Done on the User-Ladsgroup board.Feb 8 2017, 4:01 AM
thiemowmde triaged this task as Low priority.Feb 8 2017, 3:45 PM
EddieGP added a comment.EditedFeb 17 2017, 2:30 PM

This bug is a duplicate of T102013 which I've just uploaded a patch for. But as this task (the duplicate) already has a patch pending I did not merge this one (the duplicate) into the original but the other way round and withdrawn my patch for the original.
Edit: Was wrong bug number.

Change 338921 had a related patch set uploaded (by EddieGP):
CSS classes on diff/hist links at enhanced RC

https://gerrit.wikimedia.org/r/338921

EddieGP added a comment.EditedFeb 21 2017, 3:02 AM

The patch adressing Special:Contributions already got merged. Patchset 335958 by @Ladsgroup is still open for review and adresses adding the css classes at Special:RecentChanges in the old view (that e.x. comes without groups for edits to the same page). I've uploaded a patchset adding these classes to the enhanced version of RC. Here's a screenshot of it while I was developing it (I gimp'd the css definition in the corner so you know which classes these are). I wasn't sure how to handle (cur | prev). Should they get an own class or should they both stay with mw-changeslist-diff (as is now)? Feedback would be great, would change my patch if there is a opinion to give extra classes.

Change 335958 merged by jenkins-bot:
Add class in diff and history links in ChangesList

https://gerrit.wikimedia.org/r/335958

EddieGP closed this task as Resolved.Feb 23 2017, 9:44 PM

Closing here. If someone is not okay with the outcome of my questions raised in the comment before feel free to reopen and leave a comment.

EddieGP reopened this task as Open.Feb 27 2017, 8:11 AM

Sorry, my mistake, there is still one patch open for this.

Tgr added a subscriber: Tgr.Feb 28 2017, 3:26 AM

IMO there isn't really any compelling use case for this task. The original idea was to offer a way for tools which annotate change list lines (like ORES) to find the revision ID, by making out possible to figure out what the revision ID of a change is, by tagging the difflink so the tool can fetch the link URL and parse out the ID, but that's super overcomplicated and unreliable (edits which create a new page do not have any difflinks), and should be done on top of https://gerrit.wikimedia.org/r/#/c/336963/ instead.

Patrollers may want to hide the "hist" links.

Patrollers may want to use tools like https://sv.wikipedia.org/wiki/MediaWiki:Gadget-DiffDialog.js

Are these use cases good enough?

Tgr added a comment.Feb 28 2017, 7:19 PM

In the case of that specific tool, only the "diff" links would make sense, I think?

We added classes on the link to contributions, talk page, block page and email page for each user in changeslist. Users questioned for them specifically to get easier selection in css/js. This does not refer to how developers adress these elements elsewhere in the core, in skins or in extensions but is meant for Gadgets or one's own css files, i.e. I could use it in my User:EddieGP/common.css to say #mw-changeslist-history { display: none; } if I don't want to see those history links. And when editing these css files it is simply the most easy thing to do for a user to use css classes - that's what one would expect in css code. This were the reasons for T156879 to introduce classes on the user links I mentioned - I think it's pretty much the same thing here, there's not really a difference if I want to select a link which belongs to the user of a revision in changeslist or if the link is about the revision itself. The example above show's that there's a usecase for diff links and though there isn't a specific tool linked it names a usecase for hist links (some might want to hide them).

I don't think we should discuss whether a specific link is necessary (do we really need a class for the user block link? do we really need want for the mail link? wouldn't the ones for talk page and contributions be enough?) but we should be consistent and either implement classes for all of those links or do so for none of them. The disadvantages and advanteges for adding those seem to be all the same, whether it's about a block, a hist or a difflink. Could we at least agree on this?

In addition, I don't really get the disadvantages that would arise from implementing this. The advantages are clear, it would make creating custom css for users a lot easier on those links. I see that once those classes are in the code, they may be used elsewhere in core, skins or extensions to adress that specific code which as far as I understood is what you're opposing to. Of course once it those classes exist developers will very likely use it and maybe they'll use it to select things in a way we don't want. But I don't think that's reason enough to kick off css classes but instead means that we should not review code that "misuses" those classes.

What I'm totally agreeing on with you is that at enhanced RC we should not use mw-changeslist-diff for (cur | prev)-links, but that's not something about IF we should implement this but about HOW we should do so, and I think the main point we're discussing here is the "if". For the "how", I'm going to change this thing in my patch tomorrow or within the next few days.

Tgr added a comment.Feb 28 2017, 8:44 PM

If you primarily want to support user styling then you should probably add a different class (or combination of classes) to each kind of diff. (Or at least "diff" and "cur", since the third kind is in a different place so it can already be differentiated easily.)

EddieGP added a comment.EditedFeb 28 2017, 9:05 PM

That's exactly what I meant with my last sentence, I'm going to do this within the next days. So am I right with the impression that you were rather worried about the intention named in the task description but would be fine with doing this for user styling?

Tgr added a comment.Feb 28 2017, 10:17 PM

Yeah, in general if I'm rather worried about something I vote -1 :)

I've changed the patch as discussed, there is one new class for cur-links and one for prev-links. I've kept the mw-changeslist-diff class only for the difflinks that summarize a bunch of edits into one single diff.

Change 338921 merged by EddieGP:
[mediawiki/core] Add CSS classes for diff/hist links at Enhanced RC

https://gerrit.wikimedia.org/r/338921

Tgr added a comment.Mar 2 2017, 7:54 PM

I think the task as stated in the title is now resolved (although there are a few other pages that are "changes lists" but not ChangesList subclasses, such as history, deleted contributions, new pages).

EddieGP closed this task as Resolved.Mar 2 2017, 7:56 PM

Agreed