Summary

Add pop-open to the docked file index and improve scrolling behavior.

Review Request #12942 — Created April 6, 2023 and submitted April 25, 2023, 11:42 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

2605, 4059

Depends On

Reviewers

Groups

reviewboard

People

Description

This change also dramatically improves the behavior of scrolling when
anchors are clicked (filenames or chunk indicators in the file index) or
keyboard shortcuts are used to navigate. This is kind of tricky because
we have a lot of (literal) moving pieces--the page itself scrolls, and
the docked file index can change height. The height calculation inside
the file index has been exposed, and the page view runs the calculation
twice in order to dial in a good target scroll position.

Testing Done


Exercised the expansion icon a fair bit. Verified that the animation

  worked as expected and that the index was closed either when the icon

  was clicked again or when the page was scrolled (including clicking on

  an anchor to scroll to another part of the page).
Clicked on file index filenames and chunk indicators to scroll. Saw

  that the page always scrolled such that the target index was displayed

  a bit below the bottom of the banner, even when the banner was

  significantly changing size depending on the new scroll viewport.
Went through all the diffviewer keyboard shortcuts and saw that files

  and chunks were scrolled nicely.
Ran js-tests.

Commits

Summary	ID
Add pop-open to the docked file index and improve scrolling behavior. This change adds an expansion icon to the right of the file index when it's docked into the review banner. Clicking this will temporarily expand the entire index, allowing the user to quickly switch between files without having to scroll back up to the top of the page. There's a subtle animation when this is opened, though closing happens immediately. This change also dramatically improves the behavior of scrolling when anchors are clicked (filenames or chunk indicators in the file index) or keyboard shortcuts are used to navigate. This is kind of tricky because we have a lot of (literal) moving pieces--the page itself scrolls, and the docked file index can change height. The height calculation inside the file index has been exposed, and the page view runs the calculation twice in order to dial in a good target scroll position. Testing Done: - Exercised the expansion icon a fair bit. Verified that the animation worked as expected and that the index was closed either when the icon was clicked again or when the page was scrolled (including clicking on an anchor to scroll to another part of the page). - Clicked on file index filenames and chunk indicators to scroll. Saw that the page always scrolled such that the target index was displayed a bit below the bottom of the banner, even when the banner was significantly changing size depending on the new scroll viewport. - Went through all the diffviewer keyboard shortcuts and saw that files and chunks were scrolled nicely. - Ran js-tests. Bugs Fixed: 2605, 4059	a853f7d65c13fae2401aec0658bdd07ed177e963

Summary

Add pop-open to the docked file index and improve scrolling behavior.

This change adds an expansion icon to the right of the file index when it's docked into the review banner. Clicking this will temporarily expand the entire index, allowing the user to quickly switch between files without having to scroll back up to the top of the page. There's a subtle animation when this is opened, though closing happens immediately. This change also dramatically improves the behavior of scrolling when anchors are clicked (filenames or chunk indicators in the file index) or keyboard shortcuts are used to navigate. This is kind of tricky because we have a lot of (literal) moving pieces--the page itself scrolls, and the docked file index can change height. The height calculation inside the file index has been exposed, and the page view runs the calculation twice in order to dial in a good target scroll position. Testing Done: - Exercised the expansion icon a fair bit. Verified that the animation worked as expected and that the index was closed either when the icon was clicked again or when the page was scrolled (including clicking on an anchor to scroll to another part of the page). - Clicked on file index filenames and chunk indicators to scroll. Saw that the page always scrolled such that the target index was displayed a bit below the bottom of the banner, even when the banner was significantly changing size depending on the new scroll viewport. - Went through all the diffviewer keyboard shortcuts and saw that files and chunks were scrolled nicely. - Ran js-tests. Bugs Fixed: 2605, 4059

a853f7d65c13fae2401aec0658bdd07ed177e963

Files

Issues

Description	From	Last Updated
Instead of the hamburger menu icon, I prefer something that would change depending on whether the index is collapsed or …	maubin	April 24, 2023, 2:28 p.m.
Should add documentation.	maubin	April 17, 2023, 12:19 p.m.
Small nit but I think modifiers should be placed before any children in the component? Not sure if this is …	maubin	April 18, 2023, 3:38 p.m.

flake8 passed.

JSHint passed.

Docked index (collapsed) (Revision 1)

The issue has been dropped. Show all issues

Instead of the hamburger menu icon, I prefer something that would change depending on whether the index is collapsed or expanded. Something like v and ^ arrows, or maybe + and - icons like we have for other things that expand and collapse.

I would be fine with keeping the hamburger though.

david April 17, 2023, 12:19 p.m.
```
Christian, any thoughts?
```

david April 24, 2023, 2:28 p.m.

OK, plan is to land this as-is and consider future UI improvements as we go.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Should add documentation.
```

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

The issue has been dropped. Show all issues

Small nit but I think modifiers should be placed before any children in the component? Not sure if this is an actual style rule that we follow but I feel like I've seen that around a lot.

david April 17, 2023, 12:19 p.m.

It usually is, but I feel like in this case, the rules within this are only rules that apply to the children (there are no style rules for the element directly). So it's effectively:

Rules for element
Rules for children
Rules for children when element is modified

Which I think is a sensible order.

maubin April 18, 2023, 8:41 a.m.
```
Got it, that makes sense.
```

Commits:

	Summary	ID
	Add pop-open to the docked file index and improve scrolling behavior. This change adds an expansion icon to the right of the file index when it's docked into the review banner. Clicking this will temporarily expand the entire index, allowing the user to quickly switch between files without having to scroll back up to the top of the page. There's a subtle animation when this is opened, though closing happens immediately. This change also dramatically improves the behavior of scrolling when anchors are clicked (filenames or chunk indicators in the file index) or keyboard shortcuts are used to navigate. This is kind of tricky because we have a lot of (literal) moving pieces--the page itself scrolls, and the docked file index can change height. The height calculation inside the file index has been exposed, and the page view runs the calculation twice in order to dial in a good target scroll position. Testing Done: - Exercised the expansion icon a fair bit. Verified that the animation worked as expected and that the index was closed either when the icon was clicked again or when the page was scrolled (including clicking on an anchor to scroll to another part of the page). - Clicked on file index filenames and chunk indicators to scroll. Saw that the page always scrolled such that the target index was displayed a bit below the bottom of the banner, even when the banner was significantly changing size depending on the new scroll viewport. - Went through all the diffviewer keyboard shortcuts and saw that files and chunks were scrolled nicely. - Ran js-tests. Bugs Fixed: 2605, 4059	f49515ab652079e728a8561d711c37fd001d15f5
	Add pop-open to the docked file index and improve scrolling behavior. This change adds an expansion icon to the right of the file index when it's docked into the review banner. Clicking this will temporarily expand the entire index, allowing the user to quickly switch between files without having to scroll back up to the top of the page. There's a subtle animation when this is opened, though closing happens immediately. This change also dramatically improves the behavior of scrolling when anchors are clicked (filenames or chunk indicators in the file index) or keyboard shortcuts are used to navigate. This is kind of tricky because we have a lot of (literal) moving pieces--the page itself scrolls, and the docked file index can change height. The height calculation inside the file index has been exposed, and the page view runs the calculation twice in order to dial in a good target scroll position. Testing Done: - Exercised the expansion icon a fair bit. Verified that the animation worked as expected and that the index was closed either when the icon was clicked again or when the page was scrolled (including clicking on an anchor to scroll to another part of the page). - Clicked on file index filenames and chunk indicators to scroll. Saw that the page always scrolled such that the target index was displayed a bit below the bottom of the banner, even when the banner was significantly changing size depending on the new scroll viewport. - Went through all the diffviewer keyboard shortcuts and saw that files and chunks were scrolled nicely. - Ran js-tests. Bugs Fixed: 2605, 4059	a853f7d65c13fae2401aec0658bdd07ed177e963

Diff:

Revision 2 (+482 -174)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
	reviewboard/static/rb/js/reviews/views/reviewablePageView.ts
	reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-6.x (d7fa838)