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

Update URL whenever prices are filled #366

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

axismb
Copy link

@axismb axismb commented May 14, 2020

Not sure if its useful for others but I've had this issue on desktop & know at least one other friend with it too.

Basically, when you're tabbing through inputs and copy the URL from the url bar of your browser, the URL ends up being stale unless you explicitly click the Copy Permalink button. This fork automatically updates the url as you update it so you can easily Cmd/Ctrl + L and copy paste.

Not sure if its useful for others but I've had this issue on desktop & know at least one other friend with it too.

Basically, when you're tabbing through inputs and copy the URL from the url bar of your browser, the URL ends up being stale unless you explicitly click the Copy Permalink button. This fork automatically updates the url as you update it so you can easily Cmd/Ctrl + L and copy paste.
Copy link
Contributor

@peter50216 peter50216 left a comment

Choose a reason for hiding this comment

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

Should this only updates the URL if the original prices comes from URL parameters?

Currently localstorage is disabled while there are parameters from the URL, so clicking random links won't accidentally override user's local data. Would this change causes that localstorage is not used most of the time?

js/scripts.js Outdated
@@ -375,6 +375,7 @@ const update = function () {
} else {
permalink_button.hide();
}
permalink && window.history.replaceState && window.history.replaceState({}, null, permalink);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use a simple if to be consistent with other parts of the code.

@axismb
Copy link
Author

axismb commented May 14, 2020

Should this only updates the URL if the original prices comes from URL parameters?

Currently localstorage is disabled while there are parameters from the URL, so clicking random links won't accidentally override user's local data. Would this change causes that localstorage is not used most of the time?

I think, from the testing I did, this does not change that behavior, on a clean localstorage, I was able to confirm loading a url with a query param did not set localstorage (it set populated_from_query = true) and changing the inputs which in turn update did not update localstorage either.

However if I load the url without any query params, it loads from localstorage then updates the query params.

Let me know if this makes sense or if I should test more cases.

@peter50216
Copy link
Contributor

Should this only updates the URL if the original prices comes from URL parameters?
Currently localstorage is disabled while there are parameters from the URL, so clicking random links won't accidentally override user's local data. Would this change causes that localstorage is not used most of the time?

I think, from the testing I did, this does not change that behavior, on a clean localstorage, I was able to confirm loading a url with a query param did not set localstorage (it set populated_from_query = true) and changing the inputs which in turn update did not update localstorage either.

However if I load the url without any query params, it loads from localstorage then updates the query params.

Let me know if this makes sense or if I should test more cases.

The case I'm curious on is loading the url without any query params, and then whether filling values updates localstorage (I'd guess so).

But there might be another unexpected behavior from user perspective:

  • Navigates to the url without any query params, fill in some values.
  • Reloads the page (can happen by e.g. restarting the browser)
  • Fill in more values, which iiuc won't be saved to localstorage at this point after the change, since the URL is changed.
  • Close the website and found out that some values are missing the next time the site is opened.

@axismb
Copy link
Author

axismb commented May 14, 2020

Should this only updates the URL if the original prices comes from URL parameters?
Currently localstorage is disabled while there are parameters from the URL, so clicking random links won't accidentally override user's local data. Would this change causes that localstorage is not used most of the time?

I think, from the testing I did, this does not change that behavior, on a clean localstorage, I was able to confirm loading a url with a query param did not set localstorage (it set populated_from_query = true) and changing the inputs which in turn update did not update localstorage either.
However if I load the url without any query params, it loads from localstorage then updates the query params.
Let me know if this makes sense or if I should test more cases.

The case I'm curious on is loading the url without any query params, and then whether filling values updates localstorage (I'd guess so).

But there might be another unexpected behavior from user perspective:

  • Navigates to the url without any query params, fill in some values.
  • Reloads the page (can happen by e.g. restarting the browser)
  • Fill in more values, which iiuc won't be saved to localstorage at this point after the change, since the URL is changed.
  • Close the website and found out that some values are missing the next time the site is opened.

Yep, the first case you mention works correctly. That's a good point about this edge case/behavior, I wonder if checking the values indexdb across what's in the query params is a good way to deal with that; e.g, if they're all the same, continue to update localstorage.

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

Successfully merging this pull request may close these issues.

None yet

3 participants