1075222 - Prevent orphan opening/closing parens on new job rows

Status: RESOLVED INCOMPLETE

(Tree Management :: Treeherder, defect, P4)

Description

[Jonathan French (:jfrench)]

This was raised in channel by kwierso, it might be nice if there was a way to wrap the job row elements so that we wouldn't have orphan parens on new job rows.

I will upload a couple of screen grabs to illustrate the current (occasional) appearance, vs. the requested appearance.

I think it may result in slightly higher line/space consumption when wraps are triggered earlier, but perhaps not much, even over large result sets.

Comment 1

[Jonathan French (:jfrench)]

Attachment: orphanParen

Comment 2

[Jonathan French (:jfrench)]

Attachment: requestedBehavior

See above for the current behavior, and requested behavior.

Comment 3

[Cameron Dawson [:camd]]

The fix for this would likely be in CSS, but one of these areas could be related:

https://github.com/mozilla/treeherder/blob/master/ui/index.html#L191-L222
https://github.com/mozilla/treeherder/blob/master/ui/js/directives/treeherder/clonejobs.js#L633-L659
https://github.com/mozilla/treeherder/blob/master/ui/js/directives/treeherder/clonejobs.js#L542-L576

Thanks!

Comment 4

[GitHub Autolander Bot]

Attachment: [treeherder] darkwing:1075222-orphan-parens > mozilla:master

Comment 5

[Jonathan French (:jfrench)]

I've added some comments on the PR. Should Sara no longer be the assignee for this bug?

Comment 6

[Cameron Dawson [:camd]]

Yeah, Sara was an Outreachy applicant, but she's moved to other things now, afaik.

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8820834 [details] [review]
[treeherder] darkwing:1075222-orphan-parens > mozilla:master

Yeah, Jon is totally right.  I just tested it.  We'll need to try another approach for this.  I think this is one that's much harder than it seems it should be.  :)

Comment 8

[Jonathan French (:jfrench)]

I had wondered in the past if an ng-class like 'job-group-expanded' could conditionally be applied when a user clicks to expand a specific job group, and a css wrap rule then applied which overrides the global nowrap (in the current PR). That might prevent the expanded jobs flying offscreen.

I'm not sure about the performance hit applying that class on every job group doing a global expand though. Maybe we could listen to a 2nd scope variable for global expand like 'global-job-groups-expanded' as high up as we can in a parent element, and do some css child targeting for that.

It might still be the case when locally expanding a job group, overriding its nowrap will result in it bouncing around on the page, not sure.

Comment 9

[Cameron Dawson [:camd]]

David--  did you want to take another crack at this?  It's not a huge deal by any means, but if you have some other cool CSS tricks, it'd be neat to have it working cleanly.  On the other hand: I'm fine if we "wontfix" this, since it's really just cosmetic.

Comment 10

[David Walsh :davidwalsh]

The latest PR was the last trick in my playbook, I believe.  :/  I'll let you decide if you consider this a WONTFIX.

Comment 11

[Ed Morley [:emorley]]

Let's punt for now :-)