-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clear selection on page change #1631
Clear selection on page change #1631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I tested with a myriad of module numbers, and they worked great on my end. Good job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clears the selection when using the top and bottom buttons but doesn't when using the left-hand nav menu. Also I'm weary of doing any DOM manipulation in the *-util classes (not that we're perfect about that).
Instead, I'm thinking add something to viewer-app's componentDidUpdate method. There's a method that gets called there - scrollToTopIfNavTargetChanging
- We could add a similar method where that gets called - clearSelectionIfNavTargetChanging
. (Or combine both into scrollToTopAndClearSelectionIfNavTargetChanging
, but nobody seems to like my super-long function names)
…Obojobo into issue/1618-clear-selection-on-page-change
…issue/1618-clear-selection-on-page-change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I merged in dev/16)
Awesome, approved!
Any text highlighted on a page now gets cleared when navigating to different pages using the nav button at the top/bottom of modules. For this issue, I've added a
clearSelection()
function to thenav-util
but I'm open to moving it to a different file if appropriate.Fixes #1618