Bug 1151225 - Wrap first line of commit message

[deathping1994]

[edmorley]

Thank you for taking a look at this - some comments below :-)

---

^{webapp/app/css/treeherder.css, line 384 [r1] (raw file):}
Please remove the extra whitespace at the start and end of the block :-)

---

^{webapp/app/index.html, line 127 [r1] (raw file):}
I believe the html5 spec prohibits putting a div inside a span. We can actually just use the span itself, we don't need the div as far as I can see. Let's just re-use the class 'revision-comment', but since it's also used for something in the plugin panel too, we'll need to create a new class name for the plugin panel uses as well.

---

---

Comments from the review on Reviewable.io

[edmorley]

Reviewed files:

• webapp/app/css/treeherder.css @ r1
• webapp/app/index.html @ r1

---

Comments from the review on Reviewable.io

[edmorley]

That's better thank you :-)
Could you also rename the other revision-comment instance to 'classification-comment' or similar?
https://github.com/mozilla/treeherder-ui/blob/edb92da624d3bc45baa408b9c5973d956d64fa1c/webapp/app/plugins/pluginpanel.html#L120

@tojonmz, any thoughts on whether the white-space: normal is needed etc? (CSS/making things look pretty not my forté!)

[deathping1994]

@edmorley white-space: normal; will collapse Sequences of whitespace into a single whitespace. Text will wrap when necessary. This is default anyway but it has been specified in other classes as well so I also added it.

[tojon]

@tojonmz, any thoughts on whether the white-space: normal is needed etc?

@edmorley I think only in cases where we are trying to override a 'non-default' bootstrap value, do we add a default css property to a selector. I had done this once or twice the past year, but not frequently. I'll have a look and see if I observe any difference with a local branch.

[tojon]

@deathping1994 cool, a couple things I've noticed so far, if you do a blink-test (flip quickly between the same resultset on production vs. the PR). Eg.
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e9e47135019e

Current production

PR

• a bit of unwanted white space opens up between the Author Initials container, and the commit message (marked in red) - I think that is the 5px padding in your change, it can be removed.
• we will want to preserve the 15px empty boundary as shown between the commit messages and the job table (marked in green) but since that last word is being wrapped as hidden (eg. LazyL...), we will want to drop down to the next line so it is fully visible, when it encounters that boundary.

If revision-comment has been removed from the markup, I think that entry can be removed from treeherder.css. Looking at a repo search that appears to be the case (you can ignore /dist)
https://github.com/mozilla/treeherder-ui/search?utf8=%E2%9C%93&q=revision-comment

Thank you for looking into this @deathping1994!

[deathping1994]

Thanks @tojonmz , I'll get rid of the 5px padding.
About the 15px boundary, do you want me to add 15px padding on right edge or leave it as it is.

[tojon]

About the 15px boundary, do you want me to add 15px padding on right edge or leave it as it is.

@deathping1994 we'd like to preserve the 15px empty/white boundary between the commit messages and the job table at all times, and have the commit text that hits the edge of its container, not disappear underneath that boundary - and instead wrap down to the next line if it doesn't fit.

It looks like there might be a bit of playing needed to figure out the solution.

[tojon]

I think I see what this is doing by inducing an earlier wrap, but I am not sure if it will be needed after solving the boundary behavior problem.

[tojon]

If I remove a couple of the properties included in this change:

display: inline block;
max-width: 80%;

And then override the default 15px right hand bootstrap padding on the revision-list span, I can achieve this in inspector (blue):

And it will trigger a wrap exactly when the job table boundary (the empty space to its right), hits that blue container. No part of the string is obscured, and it wraps correctly.

However we also want the wrapped line contained within the commit message 'region', not all the way to the left. So that would still need to be sorted. But hope this helps.

[deathping1994]

I've tried almost everything I could , but I've not yet been able to understand why the text gets hidden under table. But I've found some tricks like indenting the second line on wards in .commit-msg-wrap using css ::first-line pseudo element, or setting max-width to 80% for .commit-msg-wrap

[wlach]

Hey Mike, Jonathan and I had a quick look at this. I think getting this working properly using the current approach might be hard -- I tried for about 20 minutes to find some combination of things that would work, with no luck. I suspect this might be the sort of case that flexbox could handle well -- fixed width for commit sha + author, then use the rest of the space for the message. For more information on how to use it, see: https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Using_flexbox_to_lay_out_web_applications

[tojon]

@deathping1994 Hi Mike, just a heads' up, the treeherder and treeherder-ui repos were unified today (per Ed's closure above, of all -ui PR's). This means that this -ui PR would need to continue as a new PR in https://github.com/mozilla/treeherder.