Skip to content
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

Persist routerId across page reloads #162

Merged
merged 4 commits into from
Jun 22, 2020
Merged

Persist routerId across page reloads #162

merged 4 commits into from
Jun 22, 2020

Conversation

landonreed
Copy link
Member

@landonreed landonreed commented Apr 30, 2020

This PR fixes #161. otp-rr already has a provision for overriding the default router (note: this feature will be going away with otp 2.x). This simply adds persistence of this override value for the session (across page reloads, but not new tabs). It also adds a confirm message with the option to revert to the default router.

To test:

  1. yarn start
  2. Go to http://localhost:9966/#/start/40.5885995/-73.6394705/11/City_of_Long_Beach_ea05fbd9-bfc6-413a-ab04-e777b95484dd
  3. Open js console
  4. Plan trip
  5. API request should go to /otp/routers/City_of_Long_Beach_ea05fbd9-bfc6-413a-ab04-e777b95484dd
  6. 🆕 Click Copy Link button to get a URL with the routerId included. Paste into new tab and verify that trip is planned correctly.
  7. To remove override, http://localhost:9966/#/start/40.5885995/-73.6394705/11/SOME_OTHER_VALUE. And click Cancel.

@landonreed landonreed changed the title Persist routerId across sessions Persist routerId across page reloads Apr 30, 2020
@codecov-io
Copy link

Codecov Report

Merging #162 into dev will increase coverage by 0.01%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #162      +/-   ##
==========================================
+ Coverage   10.51%   10.53%   +0.01%     
==========================================
  Files         107      107              
  Lines        3796     3808      +12     
  Branches     1023     1028       +5     
==========================================
+ Hits          399      401       +2     
- Misses       2954     2961       +7     
- Partials      443      446       +3     
Impacted Files Coverage Δ
lib/actions/ui.js 19.51% <0.00%> (-0.49%) ⬇️
lib/reducers/create-otp-reducer.js 12.68% <20.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20974b3...98b4790. Read the comment docs.

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Jus a minor naming preference but otherwise good to go.

Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

Not sure if a window.confirm is necessary, also agree with @binh-dam-ibigroup on variable name change.

@evansiroky evansiroky removed their assignment May 15, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2020

Codecov Report

Merging #162 into dev will decrease coverage by 0.48%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #162      +/-   ##
==========================================
- Coverage   10.51%   10.03%   -0.49%     
==========================================
  Files         107      107              
  Lines        3796     3998     +202     
  Branches     1023     1106      +83     
==========================================
+ Hits          399      401       +2     
- Misses       2954     3119     +165     
- Partials      443      478      +35     
Impacted Files Coverage Δ
lib/actions/ui.js 20.00% <ø> (ø)
lib/reducers/create-otp-reducer.js 12.68% <20.00%> (+0.26%) ⬆️
lib/util/itinerary.js 0.00% <0.00%> (ø)
lib/components/form/styled.js 0.00% <0.00%> (ø)
lib/components/mobile/main.js 0.00% <0.00%> (ø)
lib/components/app/print-layout.js 0.00% <0.00%> (ø)
lib/components/mobile/search-screen.js 0.00% <0.00%> (ø)
lib/components/form/settings-preview.js 0.00% <0.00%> (ø)
lib/components/mobile/results-screen.js 0.00% <0.00%> (ø)
lib/components/app/default-main-panel.js 0.00% <0.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20974b3...22c40ae. Read the comment docs.

if (routerId) {
const parts = url.split('#')
if (parts.length === 2) {
url = `${parts[0]}#/start/x/x/x/${routerId}${parts[1]}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I like this magic URL fragment!

@binh-dam-ibigroup binh-dam-ibigroup removed their assignment Jun 19, 2020
@landonreed landonreed merged commit b27e9a6 into dev Jun 22, 2020
@landonreed landonreed deleted the store-router-id branch June 22, 2020 16:59
@landonreed landonreed restored the store-router-id branch June 22, 2020 19:40
landonreed added a commit that referenced this pull request Jun 22, 2020
@landonreed
Copy link
Member Author

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alternative routerId value disappears after page reload
5 participants