Skip to content

Conversation

@interim17
Copy link
Contributor

@interim17 interim17 commented Dec 5, 2024

Time estimate or Size

small

Problem

Closes #617
If you are at /viewer hitting back won't actually load the previous trajectory. Same for forward, bug reproduces with local files as well (i.e. navigating back to networked trajectory URL after loading local file).

Solution

Added a new useEffect and handlePopState to useLocationChange

Didn't work with the [location] dependency of previously existing useEffect so made a new one.

@interim17 interim17 added the bug Something isn't working label Dec 5, 2024
@interim17 interim17 assigned interim17 and unassigned interim17 Dec 5, 2024
Comment on lines +14 to +22
import { APP_ID, URL_PARAM_KEY_FILE_NAME } from "./constants";
import TRAJECTORIES from "./constants/networked-trajectories";
import { createReduxStore } from "./state";
import { setIsPlaying } from "./state/viewer/actions";
import {
changeToNetworkedFile,
clearSimulariumFile,
} from "./state/trajectory/actions";
import { getUrlParamValue } from "./util/userUrlHandling";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a touch of organizing

@github-actions
Copy link

github-actions bot commented Dec 5, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 67.25% 735/1093
🟡 Branches 66.86% 113/169
🔴 Functions 36.36% 100/275
🟡 Lines 65.67% 656/999

Test suite run success

139 tests passing in 8 suites.

Report generated by 🧪jest coverage report action from 76bf2a4

@interim17 interim17 marked this pull request as ready for review December 5, 2024 01:44
@interim17 interim17 requested a review from a team as a code owner December 5, 2024 01:44
@interim17 interim17 requested review from toloudis and tyler-foster and removed request for a team December 5, 2024 01:44
Comment on lines +53 to +55
const trajectoryId = getUrlParamValue(
window.location.href,
URL_PARAM_KEY_FILE_NAME
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already getting the right URL into the address bar, just not loading/clearing.

Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this is working code... I had to google popstate event. do we use pushstate events too?

@interim17
Copy link
Contributor Author

I'm assuming this is working code... I had to google popstate event. do we use pushstate events too?

popstate is a type of event, pushState is a method that triggers a popstate event. So yes, we call pushState explicitly in some places, but popstate events can be triggered other ways, like hitting the back button, and this PR is about handling those events more explicitly

Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@interim17 interim17 merged commit e5da81a into main Dec 23, 2024
6 checks passed
@interim17 interim17 deleted the fix/popstate branch December 23, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

forward/back is buggy

4 participants