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.

Sign up for GitHub

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

Jump to bottom

Demo: auto-select offline copy of asset after storing #1001

Merged
merged 3 commits into from
Sep 5, 2017
Merged

Demo: auto-select offline copy of asset after storing #1001

merged 3 commits into from
Sep 5, 2017

Conversation

beaufortfrancois
Copy link
Contributor

FIX: #996

joeyparrish
joeyparrish reviewed Aug 30, 2017
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Looks great otherwise!

demo/offline_section.js
@@ -201,7 +210,7 @@ shakaDemo.refreshAssetList_ = function() {
group.removeChild(group.firstChild);
}

shakaDemo.setupOfflineAssets_();
return shakaDemo.setupOfflineAssets_();
Copy link
Member

Choose a reason for hiding this comment

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

Since this returns a value now, please update the annotations with @return {!Promise}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in e3af7f1

@joeyparrish joeyparrish added type: enhancement New feature or request and removed needs triage labels Aug 30, 2017
@joeyparrish joeyparrish self-assigned this Aug 30, 2017
@joeyparrish joeyparrish added this to the v2.3.0 milestone Aug 30, 2017
@beaufortfrancois
Copy link
Contributor Author

@joeyparrish We should be good now ;)

@joeyparrish
Copy link
Member

Thanks! Doing a test pass on the build bot now.

@shaka-bot
Copy link
Collaborator

Test Failure:

Generating Closure dependencies...
Running Closure linter...
195 files checked, no errors found.
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated [email protected]: Use uuid module instead
npm WARN deprecated [email protected]: ReDoS vulnerability parsing Set-Cookie https://nodesecurity.io/advisories/130
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^1.0.0 (node_modules/chokidar/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
Running htmlhint...

Config loaded: /var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/.htmlhintrc

Config loaded: /var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/.htmlhintrc

Config loaded: /var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/.htmlhintrc

Scanned 3 files, no errors found (42 ms).
Checking that the build files are complete...
Checking the tests for type errors...
/var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/demo/offline_section.js:184: ERROR - Violation: Using "Element.children" is not allowed: use Node.childNodes instead
for (var i = 0; i < group.children.length; i++) {
^^^^^^^^^^^^^^

/var/lib/jenkins/jobs/Manual PR Test (local-tests)/workspace/demo/offline_section.js:185: ERROR - Violation: Using "Element.children" is not allowed: use Node.childNodes instead
var option = group.children[i];
^^^^^^^^^^^^^^

2 error(s), 0 warning(s), 96.1% typed
Build failed
END-BUILD: FAILURE

@joeyparrish
Copy link
Member

There are a couple of build failures about children vs childElements (children attribute is not present on all platforms we support).

I also did some manual testing of your change locally, and it's not working correctly for me. After storing something, the store button goes gray and the help text says The asset is stored offline. Checkout the "Offline" section in the "Asset" list. The URI hash has changed to asset=offline:0, but the original asset is still selected in the drop-down list itself. Perhaps you're missing some call to update the list to match?

@joeyparrish
Copy link
Member

Sorry, not childElements. I meant to say childNodes.

@joeyparrish
Copy link
Member

And now it's working for me locally. Maybe I had something cached. Sorry for the noise.

@beaufortfrancois
Copy link
Contributor Author

I believe it is now fixed. @joeyparrish WDYT?

@joeyparrish
Copy link
Member

Looks good. Running tests now. Thanks!

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 69b1ce6 into shaka-project:master Sep 5, 2017
@joeyparrish
Copy link
Member

Merged. Thanks for your contribution!

@joeyparrish
Copy link
Member

I am planning to cherry-pick this to v2.2.2.

@beaufortfrancois beaufortfrancois deleted the select-offline-asset-automatically branch September 6, 2017 07:22
joeyparrish pushed a commit that referenced this pull request Sep 26, 2017
@beaufortfrancois @joeyparrish
Demo: auto-select offline copy of asset after storing (#1001)
6b9a833 
Closes #996
@joeyparrish
Copy link
Member

This has been cherry-picked and will be released in v2.2.2.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish left review comments

Assignees

@joeyparrish joeyparrish

Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
v2.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@beaufortfrancois @joeyparrish @shaka-bot