add anchor tag to URL and send house+term as query vars into download page #12987
Conversation
|
|
Yes, the JS interception is because the only reason for passing the term+house parameters are so that JS on the subsequent page can make client-side changes: specifically if that page has many terms, it hides all but three (and makes sure the "current" term is one of those (ideally in the middle)). And yes, we don't want And yes I agree the JS should be moved into a document-ready section, will do. |
|
@tmtmtmtm @chrismytton invitation to review please (some squashing necessary afterwards perhaps, but showing my working) |
| }) | ||
| }); | ||
|
|
||
| $("a.download-with-term").on("click", function(e){ |
There was a problem hiding this comment.
Such a minor thing, but not sure you need a the a in $("a.download-with-term")
There was a problem hiding this comment.
Also, if this a class that’s only used to hook JS behaviour onto, it’s good practice to prepend it with js-, so that it’s more obvious what the class is for, when people are refactoring code. So that’d make the selector look like: $('.js-download-with-term').
|
Looks good to me. As discussed in Slack, having a download page for each legislature probably makes most sense in the long run, for a number of reasons. I've created a new ticket for that at #13599. But this is fine for making the existing download page a little better in the meantime. |
|
@davewhiteland if it's good enough for Zarino, it's good enough for me :) I know it's not new to this PR, but I'm a little scared by that whole |
intercept clicks on the download button that are for specific terms, and update the href to include the house and term query variables. I'm updating the href in place and then letting the click bubble through (that is, not using $(e.target).preventDefault() here, which seems to work OK.
Implemented by proposing a `page_init_function` variable that `main.js` executes if it contains a function.
There "should" never be query variables in the href in the link to the download page (because we know that causes problems for wget elsewhere) but actually when going _back_ to an existing page (after the JS has injected them) this results in the query vars being added _again_ which breaks them. (Safari browser does this). So simply checking that there's no "?" in the href is the safety-check on this: don't add query vars if the URL already has a ? in it.
even if all the terms are being displayed, so move this check out of the conditional behaviour that hides the terms
updating the href was silly because it introduced the problem with old pages (i.e., after back button) having JS-changed href attributes, when all we want to do is jump to the new location.
because that's the convention in other names in this project to date
thanks @zarino for pointing this out :-)
thanks @zarino for pointing this out (also dropped superfluous `a` selector).
needed to send a jQuery element in, not a selector which only looks at the first element. Thanks @tmtmtmtm for help with this!
don't need to check for non-null value of wanted_house since it won't match house_slug if it's empty because house_slug is always non-empty since a house always has a name.
fixed the sloppy bounds calculations, so slice can be used
since this isn't just about terms: moved core function into main.js so it can be called from wherever it's needed (currently just download page's terms).
This makes things a lot clearer, and the single case that's being generalised here is such a list.
Also change button from anchor tag (since we don't need an href) to button. Simplify click handler by attaching it directly to the button, no need to pass target <ul> id via a data attribute. Fix some comments.
davewhiteland
force-pushed
the
issues/11911-download-queryvars
branch
from
b29e466
to
fcd0467
Compare
Anchor tag added for jumping to the right legislature.
Intercept clicks with a specific term to target using Javascript, so it works fine but not when we're running wget.
Closes #11911