Improve centering of the collapsed button in expanded diffs.

Review Request #13233 — Created Aug. 23, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

We try to vertically center the diff collapse button in the viewable
content area of an expanded region, but there were some issues with the
logic:

  1. If there's a banner, it reduces the available content area. It's then
    possible for the button to appear below the banner. This is the case
    with the Unified Banner.

  2. The button only vertically centered in relation to the
    expanded/collapsed tbody. Ajacent tbodies of the same type (e.g.,
    "equals") weren't considered, and this made the button appear shifted
    up or down.

  3. The button didn't smoothly jump to new positions until it reached
    fixed position mode (which is when we know we're centering it
    absolutely vertically in the viewport). This had a pretty bad feel to
    it.

The new design fixes all of these issues.

We now factor in the new content viewport to shift the available space
by the Unified Banner's height, ensuring there's never an overlap.

We can now specify a top, bottom, and parent for the element when
centering, offsetting the position as appropriate. DiffReviewableView
will set these based on the types of adjacent tbodies. This gives us teh
entire span of lines of a type as the boundaries for the centering.

Position updates are smoother, now using requestAnimation() rather
than a delay for repositioning. We also avoid setting more CSS
properties than we need to at once, and we do a better job of capping
the button in the contained area.

Unit tests pass.

Tested fixed-based vertical centering when the viewport spans an entire
expanded area.

Tested absolute-based vertical centering at the top and bottom of an
expanded area.

Tested that it stops at the top-most and bottom-most boundaries and does
not overflow.

Tested that it adjusts based on the scrolling of the Unified Banner.

Tested that it appears smooth and not jumpy in all cases.

Summary ID
Improve centering of the collapsed button in expanded diffs.
We try to vertically center the diff collapse button in the viewable content area of an expanded region, but there were some issues with the logic: 1. If there's a banner, it reduces the available content area. It's then possible for the button to appear below the banner. This is the case with the Unified Banner. 2. The button only vertically centered in relation to the expanded/collapsed tbody. Ajacent tbodies of the same type (e.g., "equals") weren't considered, and this made the button appear shifted up or down. 3. The button didn't smoothly jump to new positions until it reached fixed position mode (which is when we know we're centering it absolutely vertically in the viewport). This had a pretty bad feel to it. The new design fixes all of these issues. We now factor in the new content viewport to shift the available space by the Unified Banner's height, ensuring there's never an overlap. We can now specify a top, bottom, and parent for the element when centering, offsetting the position as appropriate. `DiffReviewableView` will set these based on the types of adjacent tbodies. This gives us teh entire span of lines of a type as the boundaries for the centering. Position updates are smoother, now using `requestAnimation()` rather than a delay for repositioning. We also avoid setting more CSS properties than we need to at once, and we do a better job of capping the button in the contained area.
890e22b7e007bf099f0a16dd14bcfcb7d3186d88

maubin
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (e201495)
Loading...