-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add engines versions to the URL and logic change #8
Conversation
lukasz-rutkowski-red
commented
Jul 4, 2022
•
edited
Loading
edited
- Engines versions now are visible in the URL if they are not default.
- Engine change now is not permanent, if the testsuite tab is changed, engine version will be set to the default.
src/engineChange.js
Outdated
engine.defaultVersion = window.localStorage["engine_" + engineId]; | ||
// if engine version is provided but it does not exist in 'EngineVersions' dict, replace it in url to the default value: | ||
if (engine.versions[queryVariable] === undefined) { | ||
window.localStorage["engine_" + engineId] = engine.defaultVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local Storage is mechanism for storing data in browser session, so it is can be retrieved after window reload: https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage
Right now the "engine_*" parameter is always either read from URL or default, so I believe we can drop usage of Local Storage in this context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using of Local Storage dropped.
src/engineChange.js
Outdated
window.localStorage["engine_" + engineId] = queryVariable; | ||
window.location.href = window.location.search.replace("engine_" + engineId + "=" + queryVariable, ""); | ||
} else if (window.localStorage["engine_" + engineId] === undefined) { | ||
// if engine version is missing, add the default value from dict 'EngineVersions' to the url: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about if no "engine_" URL is provided, then just use default version and do not set these URL parameters?
I believe that would be the simplest scenario from user POV - it works by default and URL parameters are used only if its required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameters are not visible in the URL now, when default parameters are chosen.
src/engineChange.js
Outdated
var version = window.localStorage["engine_" + engineId]; | ||
if (engine.versions[version] !== undefined) { | ||
engine.defaultVersion = window.localStorage["engine_" + engineId]; | ||
// if engine version is provided but it does not exist in 'EngineVersions' dict, replace it in url to the default value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please print a console warning/error in such case like:
Shaka version x.x.x is not supported, overriding with x.x.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console warning added.
38de9f7
to
5e0b009
Compare