Update demo URL with pushState() #4327
Conversation
That way people won't have to remember to copy the permalink.
|
I'm in favor of this idea. Any maintainers opposed or see areas for potential confusion? |
There was a problem hiding this comment.
Looks great, but please could you update it so that the url isn't written for the initial load (without any query params)?
At the moment it causes a new back entry that behaves like an instant redirect when you load the page the first time. Also I think for the default config, it might be better to not bake that in unless the user specifically wants to share a url with the (current) default config.
Co-authored-by: Tom Jenkinson <tjenkinson@users.noreply.github.com>
We can't just check if we're on baseURL, because then it'd never activate. I don't immediately see a better method, give or take perhaps where you set the variables.
demo/main.js
Outdated
| @@ -1459,6 +1462,7 @@ function getURLParam(sParam, defaultValue) { | |||
| return defaultValue; | |||
| } | |||
|
|
|||
| let firstLoad = true; | |||
There was a problem hiding this comment.
This works but I think it would be a bit nicer without the global variable. Maybe onDemoChanged could take a firstLoad param instead, and then this would be true when called here?
Line 340 in c8340b8
There was a problem hiding this comment.
Sure, if you prefer.
That way people won't have to remember to copy the permalink. Also you can go back to a previous configuration by pressing the back button.
This PR will...
Make it so the URL in the addressbar is the same as the permalink on the page.
Why is this Pull Request needed?
It's happened more than once that someone sent me the link to have to do it again. ;-)
Are there any points in the code the reviewer needs to double check?
Resolves issues:
Checklist