Improve usability, styling, and stability of the diff commits list.

Review Request #13334 — Created Oct. 11, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

We show a list of commits in the review request info box, the change
description boxes, the diff viewer for normal diffs, and the diff viewer
for interdiffs.

Each of these were essentially a different take on the commits list. We
three templates (2 as Django templates, 1 in JavaScript), and they were
not consistent. They didn't all agree on the same class names or
structure.

Many of the CSS class names used either piggy-backed on other styles or
conflicted with them. Some of those were deeply-scoped class names, and
some were just accidentally referencing other classes.

This broke recently with some of the work for fixing up the review
request layout and field definitions. Rather than trying to make another
attempt at reconciling all the templates and working around CSS classes,
this change fixes this up for good.

We now have a single documented CSS component for the commits list,
carefully structuring namespaced CSS classes covering all our commits
list uses.

The two Django templates have been merged into one. That means we now
have only one Django template, one JavaScript template to maintain. This
is still one more template than I'd like, but it does simplify things,
and both use the new namespaced classes to avoid future issues.

Some behavioral changes with the old list have been fixed. We no longer
maintain our wonky, non-accessible expand/collapse logic. Instead, we
use a <details> tag, which gives us exactly what we want and allows
screen readers to grab the summary and the commit description
separately. This looks, feels, and styles better than what we had
before.

We also have the ID column once again. The original commits list had
this, but it was removed for some reason, and that made this list far
less useful for reconciling commits. It's been brought back, and shows
the ID in a shortened form, with the full ID shown in a tooltip.

Ultimately, this deletes more code than it adds, simplifies all the
current logic, makes it easier to reason about, and removes all style
conflicts.

Tested every scenario that the commits list appears in, with all possible
states, to be absolutely sure there are no style issues or regressions.

Tested at various browser widths. There's more work to do for mobile, but
not for 6.0. It's better now than it was.

Tested the columns with large amounts of data, making sure they truncate
correctly.

Tested expanding and collapsing, and tested with commits with and without
a commit body.

Tested the accessibility tree. It's not perfect, but is better than before.

All Python and JavaScript unit tests pass.

Summary ID
Improve usability, styling, and stability of the diff commits list.
We show a list of commits in the review request info box, the change description boxes, the diff viewer for normal diffs, and the diff viewer for interdiffs. Each of these were essentially a different take on the commits list. We three templates (2 as Django templates, 1 in JavaScript), and they were not consistent. They didn't all agree on the same class names or structure. Many of the CSS class names used either piggy-backed on other styles or conflicted with them. Some of those were deeply-scoped class names, and some were just accidentally referencing other classes. This broke recently with some of the work for fixing up the review request layout and field definitions. Rather than trying to make another attempt at reconciling all the templates and working around CSS classes, this change fixes this up for good. We now have a single documented CSS component for the commits list, carefully structuring namespaced CSS classes covering all our commits list uses. The two Django templates have been merged into one. That means we now have only one Django template, one JavaScript template to maintain. This is still one more template than I'd like, but it does simplify things, and both use the new namespaced classes to avoid future issues. Some behavioral changes with the old list have been fixed. We no longer maintain our wonky, non-accessible expand/collapse logic. Instead, we use a `<details>` tag, which gives us exactly what we want and allows screen readers to grab the summary and the commit description separately. This looks, feels, and styles better than what we had before. We also have the ID column once again. The original commits list had this, but it was removed for some reason, and that made this list far less useful for reconciling commits. It's been brought back, and shows the ID in a shortened form, with the full ID shown in a tooltip. Ultimately, this deletes more code than it adds, simplifies all the current logic, makes it easier to reason about, and removes all style conflicts.
6ddead306e5553f3c7870defe161ec4a1e7e645a

Description From Last Updated

Any chance we can valign this?

daviddavid
david
  1. 
      
  2. Any chance we can valign this?

    1. Yep, I actually fixed that locally after putting this up for review.

  3. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. Ideally these two labels would be more aligned, but we have this problem for most of the fields here. There's a lot I want to do for better presentation in this box, but that'll be post-6.0 sometime.

  3. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (96020fe)
Loading...