Summary

Fix word wrapping in review request details fields.

Review Request #5951 — Created June 7, 2014 and submitted July 19, 2014, 10:52 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-2.0.x

Bugs

3371

Depends On

Commit

2652463...

Reviewers

Groups

reviewboard

People

Description

Long strings in the review request details fields (on the right-hand side of
the fields) would increase the width of the entire table, eventually
overlapping the main fields in a very ugly way. This change adds a max-width to
the field spans, and specifies a word-wrap to force browsers to insert line
breaks in the middle of words in the case that it can't break at a natural
place.

Testing Done


Set a branch that was just one long unbroken string of characters and saw it

  wrap at the width of the details pane.
Checked that a bunch of comma-separated bug numbers still wrapped at an ideal

  place in the text.

Issues

Description	From	Last Updated
Where does 12em come from? Does anything else use it? We should make this a constant somewhere.	chipx86	July 16, 2014, 5:49 p.m.
Can you add a comment saying that any changes to this need to be reflected in _resizeLayout in reviewRequestEditorView.js?	chipx86	July 19, 2014, 3:12 p.m.
Can we stick with: /* * this multi-line * comment format? */	chipx86	July 19, 2014, 10:52 p.m.
We should fetch the tbody first, and do further filters from that.	chipx86	July 19, 2014, 3:13 p.m.
How does this look if the value is wrapped to the following line?	chipx86	July 19, 2014, 3:11 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/css/reviews.less

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/css/reviews.less

reviewboard/static/rb/css/reviews.less (Diff revision 1)

The issue has been resolved. Show all issues

Where does 12em come from? Does anything else use it?

We should make this a constant somewhere.

david June 8, 2014, 2:25 p.m.

It's really just something I reverse engineered. There's no good way to determine the width of the second column without computing it at runtime in javascript.

chipx86 June 9, 2014, 12:22 a.m.

Silly CSS :(

Since the span is in the second column of the table, it should be constrained to that <td> anyway. The real problem is if the details table gets wider with more content. I think, instead of setting the max-width of the span, we should just be able to set the max width of the table itself.

That'll be necessary when there's custom columns with labels wider than what we use by default as well, or when dealing with localization.

david June 9, 2014, 12:38 a.m.

Setting a max-width on the table doesn't work. :(

chipx86 June 9, 2014, 12:47 a.m.

What if you put the table in a container element that has max-width?

I remember tables are pretty weird in how they handle things like min/max. Again, silly CSS...

david June 9, 2014, 12:52 a.m.

I'll keep poking but initial experiments are less than promising.

david June 18, 2014, 5:37 p.m.

After banging my head on this for the last week, I'm going to say the hard-coded 12em is the least bad solution =/

chipx86 June 19, 2014, 12:52 a.m.

Okay. I'm going to want to play with it a bit too, but failing that, the 12em sounds okay..

Change Summary:

Switch to computing max-width on the client side.

Commit:

2f18c6a4bda627b376d7bd19edd0f5cbd19f7e85

0c8e9e4e2496e3e2b4a2c07cb23675917a95ed83

Diff:

Revision 2 (+18 -9)

Show changes

	reviewboard/static/rb/css/reviews.less
	reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/static/rb/css/reviews.less (Diff revision 2)

The issue has been resolved. Show all issues

Can you add a comment saying that any changes to this need to be reflected in _resizeLayout in reviewRequestEditorView.js?

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
We should fetch the tbody first, and do further filters from that.
```
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)
The issue has been dropped. Show all issues
```
How does this look if the value is wrapped to the following line?
```
1. david July 19, 2014, 3:13 p.m.
  Just as bad? Maybe worse.

Commit:

0c8e9e4e2496e3e2b4a2c07cb23675917a95ed83

2652463f07fcada32810cb61d4eb5fdcbe4ad871

Diff:

Revision 3 (+21 -9)

Show changes

	reviewboard/static/rb/css/reviews.less
	reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Ship it!

reviewboard/static/rb/css/reviews.less (Diff revisions 2 - 3)

The issue has been resolved. Show all issues

Can we stick with:

/*
 * this multi-line
 * comment format?
 */

Status:: Completed
Change Summary:: Pushed to release-2.0.x (c6205d8)