Fix issue 386 #395
Fix issue 386 #395
Conversation
|
I'm targeting this PR to |
|
@jonasjabari if it is in master, how will you backport it to dev? The more normal strategy (that I'm used to) is merge it to develop, then cherry pick and adapt it for the release branch support.
|
| if (isNavigatingToAnotherPage(document.location, event)) { | ||
| this.$store.dispatch("navigateTo", {url: document.location.pathname, backwards: true} ); | ||
| }; | ||
| let needToNavigate = self.currentPathName !== document.location.pathname || |
There was a problem hiding this comment.
the intent behind extracting it to a separate file was to encapsulate the logic, make it independently testable in JS unit tests and not muddy the app.js with these implementation details. I'd still pefer that personally, otherwise I'd vote for a private function to take care of this as I think the implementation detail here makes it harder to read this function/see what it's actually doing. But that's arguably style (aka my style is small intention revealing functions that point you to the detail)
There was a problem hiding this comment.
changed that to your approach ;)
| expect(page).to have_content("This is Page 2") | ||
| expect(page).to have_selector("body.not-reloaded") | ||
|
|
||
| # page.evaluate_script('window.history.back()') |
There was a problem hiding this comment.
please delete outcommented code :)
| page.go_back | ||
|
|
||
| element = page.find("#my-div-on-page-1") | ||
| refreshed_content_on_page_1 = element.text |
There was a problem hiding this comment.
This is prone to a race condition again. if page.go_back needs longer than finding the div then it might end up with the old content. You can fix this with expect(page).to have_no_content(first_content_on_page_1) (expect(page).not_to have_content... also works due to dark rspec magic) instead of expect(first_content_on_page_1).not_to eq(refreshed_content_on_page_1)
| @@ -10,12 +9,20 @@ const componentDef = { | |||
| }, | |||
| computed: Vuex.mapState({ | |||
| asyncTemplate: state => state.pageTemplate, | |||
| currentPathName: state => state.currentPathName, | |||
| currentSearch: state => state.currentSearch, | |||
| currentOrigin: state => state.currentOrigin, | |||
There was a problem hiding this comment.
I'm a bit confused that we'd need this. Yes, I read your comment it seems weird to me that there should be no way to access that state throughout the history/event API etc. 🤔
And I mean I also tried it out (not with the back button though obviously 😅 ) and there I had it.
There was a problem hiding this comment.
Hmkay, seems more involved. Odd, I guess I have to read up on the history APIs etc. https://stackoverflow.com/questions/8980255/how-do-i-retrieve-if-the-popstate-event-comes-from-back-or-forward-actions-with
There was a problem hiding this comment.
I tried a lot, but this was the only way to fix it.
I change target branch to |
| if (isNavigatingToAnotherPage({ | ||
| origin: self.currentOrigin, | ||
| pathName: self.currentPathName, | ||
| search: self.currentSearch |
There was a problem hiding this comment.
Small thing, having all these "flat" in the store seemed weird to me before. This makes me feel it's even weirder. How about storing them in an object in the store? Like: currentLocation: {all_da_values} then that object could be passed on as a whole. It also makes it clearer that all these values belong together and are not of too much use on their own.
Issue #386: page transition is broken when using browser back button
Changes