Rework page navigation state for the diff viewer.
Review Request #9717 — Created March 2, 2018 and submitted
We use
Backbone.Router
and URLs to set the current view of the diff
viewer, controlling which revisions and pages are shown. Currently, this
is pretty basic -- we only need to worry about revisions in the path,
and a single query string option for the page. This means that any bits
of code updating this view state just needed to have the router navigate
to a new URL. Soon, though, we'll need more query string options, and we
weren't set up for this at all. This change aims to fix that.There's now a central
_navigate()
method, which takes care of setting
the URL state based on the options provided and the existing page state.
It's the source of truth on how page URLs set within the view should
look. All call sites now call into this, instead of working with URLs
themselves.The router itself now supports arbitrary query strings. This means
switching to our own regex, instead of using their capture groups (which
don't actually work well for more general usage -- there's lots of
workarounds online, but this seems to be the best option). While the
regex is a bit more complicated, the body of the handler is now a bit
simpler, and will be easier to expand with more diff loading options
going forward.
DiffViewerPageModel
was updated to use the newDjblets.buildURL()
function for constructing the URL used to load the diff context, which
will also help us with expanding usage soon.Unit tests were added that test the various navigation combinations,
both in a generic way (testing_navigate()
) and more specific ways
(testing the handlers for revision/page selection, anchor
navigation, and initial page states).
All unit tests pass.
Manually tested changing revisions (testing all combinations of single
and interdiff revisions), changing pages, navigating anchors, and setting
initial pages. The page state and the current URL always reflected what I
expected to see.
Description | From | Last Updated |
---|---|---|
Col: 35 ['page'] is better written in dot notation. |
reviewbot | |
Col: 37 Expected '===' and instead saw '=='. |
reviewbot | |
Col: 26 ['page'] is better written in dot notation. |
reviewbot | |
Instead of having these be optional parameters which are only used in unit tests, how about changing the only real … |
david |
- Change Summary:
-
- Switched to using
queryArgs.page
instead ofqueryArgs['page']
. - Fixed a
==
that should have been a===
. - Changed query string data to be built in a specific order.
- Switched to using
- Commit:
-
60eacb7e53b9873f47bb9fd9a77c9f1118a2623b503175e8d0b4786fa8e5f2ef7dedd00ae38cca25
- Diff:
-
Revision 2 (+508 -110)
Checks run (2 succeeded)
-
-
Instead of having these be optional parameters which are only used in unit tests, how about changing the only real callsite to be:
this._setInitialURL(document.location.search || '', RB.getLocationHash());
That way the file is a few lines shorter and there's no longer any confusion about why the parameters exist when they're not used by the actual code.
- Change Summary:
-
Switched to passing arguments to the
_setInitialURL()
call instead of deciding it within the function. - Commit:
-
503175e8d0b4786fa8e5f2ef7dedd00ae38cca259772f82b1a4bd56f4427fc7b1d6d9e8c7d7521d6
- Diff:
-
Revision 3 (+497 -110)