-
Notifications
You must be signed in to change notification settings - Fork 414
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
Right Click to Navigate History #3547
Conversation
ui/component/router/view.jsx
Outdated
useEffect(() => { | ||
if (typeof title !== 'undefined' && title !== '') { | ||
document.title = title; | ||
} else if (typeof streamName !== 'undefined' && streamName !== 'undefined' && streamName !== '') { |
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.
BTW - streamName was coming as a string of value 'undefined'. I didn't look too far into this bug.
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 think it's when you call parseURI
with an undefined
uri. Only calling parseURI
if uri exists should fix it.
6fa3287
to
9e68e2c
Compare
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.
A few questions
@@ -96,7 +96,7 @@ const persistOptions = { | |||
|
|||
let history; | |||
// @if TARGET='app' | |||
history = createHashHistory(); | |||
history = createMemoryHistory(); |
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.
What is the difference between these two?
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.
createMemoryHistory gives us an array of entries so that we can reference the history stack.
Excerpt from history/docs/GettingStarted
Additionally, createMemoryHistory provides history.index and history.entries properties that let you inspect the history stack.
const { entries } = history; | ||
const entryIndex = history.index; | ||
|
||
useEffect(() => { |
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.
Why did you move this effect out of the show component?
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 want this to fire on every page and not just the pages that render using the ShowPage component.
const slicedEntries = sliceEntries(currentIndex, entries, historyLength, isBackward); | ||
return ( | ||
<div> | ||
<Button |
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.
Are you able to determine if we can go back here or not? If so, it would be great to disable the back button and give it some disabled style so it's easier to tell that you can't
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.
PR has been updated to disable button if no historical entries.
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.
Awesome!
Thanks for the PR! Once this is fully reviewed/corrected/merged, please send us an email so we can show you some appreciation |
Nice work @dalhill. This is really close. I'll spend some time testing your branch this week just to make sure there aren't any issues on electron moving to memoryHistory, but I don't think there will be. I'll also push up a commit and do another pass at the styling. |
9dd5bc8
to
51bc090
Compare
Pushed to resolve conflicts |
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 pushed up a commit that cleaned up the styling. I'll get this merged today!
ui/component/router/view.jsx
Outdated
useEffect(() => { | ||
if (typeof title !== 'undefined' && title !== '') { | ||
document.title = title; | ||
} else if (typeof streamName !== 'undefined' && streamName !== 'undefined' && streamName !== '') { |
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 think it's when you call parseURI
with an undefined
uri. Only calling parseURI
if uri exists should fix it.
…itle update hook for other pages
8063419
to
ef3a316
Compare
PR Checklist
PR Type
Fixes
Issue Number: #3474
What is the current behavior?
In order to navigate history, the user must do so one step at a time.
What is the new behavior?
User is able to right click the navigation buttons to move forward or backward multiple steps.
Other information
Possible improvements