Improve step by step component double dot problem solving code #473
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
andysellick
changed the title
andysellick
force-pushed
the
step-nav-double-dot-highlight-first-active-link-and-step
branch
from
5fcb418
to
f659fac
Compare
andysellick
force-pushed
the
step-nav-double-dot-highlight-first-active-link-and-step
branch
from
f659fac
to
e1ae137
Compare
andysellick
force-pushed
the
step-nav-double-dot-highlight-first-active-link-and-step
branch
from
e1ae137
to
75164f9
Compare
leenagupte
reviewed
Aug 7, 2018
|
|
||
| The current page in the step by step navigation is an anchor link to the top of the page. If there are more than one of these and the user clicks one that is not currently highlighted, that one will be highlighted. | ||
| If a user has not clicked a link (i.e. has visited the page without first clicking on a step by step navigation) only the first duplicate link will be highlighted. If that link is not in the active step, the JS makes the highlighted link's parent the active step. If there is no active step, the first active link will be highlighted (but there should always be an active step). |
There was a problem hiding this comment.
If that link is not in the active step, the JS makes the highlighted link’s parent the active step
How would this be possible?
There was a problem hiding this comment.
The code basically goes...
if we've got the same link twice in the component
AND
there's nothing in the session to tell us which link to highlight
AND
the first highlighted link is not in the active step
THEN
change the active step to be the one that the first highlighted link is in
There was a problem hiding this comment.
It's the thing I was talking about where the frontend code no longer trusts the backend to be right about which step should be highlighted and expanded, and takes matters into its own hands.
andysellick
force-pushed
the
step-nav-double-dot-highlight-first-active-link-and-step
branch
from
75164f9
to
92d856d
Compare
andysellick
force-pushed
the
step-nav-double-dot-highlight-first-active-link-and-step
branch
from
92d856d
to
47cd87e
Compare
DilwoarH
reviewed
Aug 14, 2018
|
|
||
| if (lastClicked) { |
There was a problem hiding this comment.
should you keep this in? What would happen if the active class couldn't be found either?
There was a problem hiding this comment.
If the active class isn't found then this entire function has already exited on line 231. The rest only gets called if there's more than one active class found.
|
Other than that one comment, it looks good 👍 |
DilwoarH
approved these changes
Aug 15, 2018
- shorten - if no sessionStorage value (i.e. user is arriving at the page cold) highlight the first active link and active step, not the last - expand documentation to clarify how this behaves
andysellick
force-pushed
the
step-nav-double-dot-highlight-first-active-link-and-step
branch
from
47cd87e
to
45b1124
Compare
andysellick
deleted the
step-nav-double-dot-highlight-first-active-link-and-step
branch
Merged
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.
See the documentation for full details. Expands the solution to this problem:
Trello card: https://trello.com/c/LTzIkmOX/776-optimise-default-open-step
Component guide for this PR:
https://govuk-publishing-compon-pr-473.herokuapp.com/component-guide/