701932 - [shipping] signoffs summary links don't work or do anything

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P3)

Description

[Peter Bengtsson [:peterbe]]

On the Sign-offs page (e.g. /shipping/signoffs/ar/fx8) there's the signoff summary and it contains links that look something like this::

 <p>There is a <a href="#639836d5a82c">current accepted sign-off</a>

Clicking them does nothing. Ideally, since this ID is in the table as a data-push attribute we could use Javascript to highlight clicked anchor.

Comment 1

[Axel Hecht [:Pike]]

It does scroll to the push with that revision as tip, if present. Sadly, this doesn't always exists thanks to our data migration stunts, which is also breaking the diff markers, i.e., only one of them showing up on the page.

Also, there should be a visual high-light that this is the accepted sign-off, but that's also broken, because ... .

That'd all be data1.1, really.

Comment 2

[Staś Małolepszy :stas]

(In reply to Peter Bengtsson [:peterbe] from comment #0)

> Clicking them does nothing. Ideally, since this ID is in the table as a
> data-push attribute we could use Javascript to highlight clicked anchor.

What about using CSS's :target to highlight the row of the linked changeset?  Might be an easy win for right now, while we're waiting for data1.1.

Comment 3

[Peter Bengtsson [:peterbe]]

:target? Hmm... Not sure if that would work. The anchor doesn't necessarily exist as an ID. Its in the html table as a data attribute. 

Either way, I'm sure we can figure something out to make it nice. It doesn't have to be fancy.

Comment 4

[Peter Bengtsson [:peterbe]]

Attachment: This might be pleasant

Because it's hard to see what this does, I suggest you fetch my branch and try it locally.

1. install
https://github.com/peterbe/elmo/tree/feature/bug701932-signoff-highlights
2. Go to signoff page
eg /shipping/signoffs/ar/fx8
3. Click on any of the anchor links

Note how, the anchor links bubble through and when you load the page with a hash on it, it highlights longer.

Comment 5

[Axel Hecht [:Pike]]

Hrm. I think this bug is actually FIXED with data1.1, the links actually scroll to the rows in question. stas' comment 2 actually sounds like a hack, which, ahem, ignores that we don't scroll to the row because we don't have it. So highlighting it wouldn't have worked either ;-)

Comment 6

[Peter Bengtsson [:peterbe]]

Fixed? They're anchor links and when you click them, nothing happens. My patch at least helps to connect that sentence with what you see in the table.

Comment 7

[Axel Hecht [:Pike]]

https://l10n.mozilla.org/shipping/signoffs/ar/fx8 totally moves for the summary links, to the extent that it can with the window size I have.

If the page doesn't scroll, it doesn't scroll.

If you want to do something to indicate the currently highlighted row, 

.pushrow:target {
  background-color: red;
}

does the right thing, selector-wise.

I'm not sure we're linking to a particular revision from the team page with enough reasons to make a rather drastic visual change, though. There's still the indicators of the sign-offs actions, and the suggested sign-off has the sign-off button unhidden.

So as long as they're in the viewport, they're already visually jumping out.

Comment 8

[Peter Bengtsson [:peterbe]]

Note that the $.click() handler I added bubbles through so it should do what an anchor link does, i.e. scroll to put that ID element at the top of the viewport. Commonly, the problem is that there's not enough stuff on the screen to make that anchor-link-jump do what the user expects.

Comment 9

[Axel Hecht [:Pike]]

I'll resolve this one as FIXED.

The reason this bug was filed is that we had sign-off pages that had links to anchors which didn't exist in the document to begin with. And that's fixed by data1.1.

Making the link target more visually appealing wasn't the actual problem. And, tbh, I'm old enough to remember the blink tag, so, errr, no.

(Yes, taking and target-milestoning as data1.1)

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 634222 [details] [diff] [review]
This might be pleasant

r- based on my previous comment.