when download data is clicked on term table, highlight the term #14535
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
August 12, 2016 17:11
added to static page so that JS can pick them up and add them as query vars on click
So can hang a click event off it (to add term data to the URL) Caused a long line, so moved attribs onto separate lines.
Core of this should be in main.js, with much less here.
wrapped in a general function that's run from main.js at the end of all other document-ready code, to avoid race conditions.
| @@ -479,3 +479,8 @@ $(function(){ | |||
|
|
|||
| } | |||
| }); | |||
|
|
|||
| function getQueryParam(key) { | |||
There was a problem hiding this comment.
is this something you wrote yourself, or is it taken from somewhere else? (and if the latter should it have attribution?)
|
Some comments in-place. I'm a little wary of diminishing returns here, so 'TODO' comments in-place are also plausible options for some of these things… |
added 4 commits
August 15, 2016 08:43
code now more obviously checks for hash (fragment identifier in URL) and simply adds the query vars on the end of the URL if there isn't one there. We don't anticipate there being an anchor tag there any more, but it's a sneaky gotcha that would break this behaviour should one ever be added.
Turns out modifying the request on click has a race condition on the click tracking: depends which code fires first, because both are modifying the location, and the first one to execute wins. So modifying the href (by adding anchor tags) before click is simpler.
no longer handling query vars, passing them in the fragment identifier (after the hash) in the URL instead, so removing these functions
Use the .next() sibling selector instead, since the toggle button is being injected directly above the list it relates to: so can assume that the list is its next sibling (instead of using ID).
This is just in set-up of the list: an alternative approach would be to have the hidden class have `display:none` in the stylesheet. I haven't done it that way simply because hiding the elements dynamically like this means any class can be used. Since this is only dynamically controlled, I've added the prefix `js-` to the hidden class name being used.
davewhiteland
force-pushed
the
issues/14427-highlight-download-term
branch
from
213dd3e to
a34d512
Compare
makes it a teeny bit clearer what's going on. I think the function itself is a little clunky because of the problem of the targetIndex being at (or close to) either end of the list, so there's some explicit checking for that. Also it makes it clearer that this code is adding the hidden class to everything, then removing it from the innerList... would be nicer to invert this to add the class just to things not in the innerList, which might be possible for cleanliness.
|
invitation for review @tmtmtmtm |
|
It's worth mentioning perhaps that now it's neatly encapsulated, it becomes clear that the implementation I ought to want is |
|
Reverting back to @davewhiteland to work out what should happen now that this page has been split into per-legislature rather than per-country. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #14427
This code replaces the existing JavaScript (deployed but not being used because the download button is not currently passing term (and house) as query variables) with neater code.
It's solving the problem of clicking download from a term table page, and not being able to see that term amongst the terms listed on the download page in the case where there are too many: the solution is to reduce the number of terms displayed, and highlight the specific term.
The business of displaying only a few terms on the download page is now better handled by a
collapseDisplayedItemsfunction inmain.js. The code is cleaned up with suggestions and review from @zarino and @tmtmtmtm in previous PR #12987, which this PR supersedes.The
collapseDisplayedItemsfunction hides all butmaxDisplayItemsterms, if there are more thanminThresholdItems. If there's a selected term (i.e., after a click on Download data from a term page) it is included in the displayed items.Both
termandhouse(as slugs) are passed from the term download button; if either is lacking (no term is selected) then multiple terms are still collapsed on the download page, but no specific term is hidden. By default, most recent terms are shown.The highlighted term is given CSS class
download-terms__highlighted.