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

Add engines versions to the URL and logic change #8

Merged
merged 4 commits into from
Jul 5, 2022

Conversation

lukasz-rutkowski-red
Copy link
Contributor

@lukasz-rutkowski-red lukasz-rutkowski-red commented Jul 4, 2022

  • 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.

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;
Copy link
Contributor

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

Copy link
Contributor Author

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.

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:
Copy link
Contributor

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.

Copy link
Contributor Author

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.

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:
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Console warning added.

@lukasz-rutkowski-red lukasz-rutkowski-red merged commit 809746a into main Jul 5, 2022
@lukasz-rutkowski-red lukasz-rutkowski-red deleted the engine_change branch July 5, 2022 10:20
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.

2 participants