Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Conversation

@dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Oct 5, 2017

Ping @cghawthorne


This change is Reviewable

@dsmilkov dsmilkov requested a review from nsthorat October 5, 2017 13:56
@nsthorat
Copy link
Contributor

nsthorat commented Oct 5, 2017

:lgtm_strong:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


demos/homepage/index.md, line 125 at r1 (raw file):

        </div>
      </a>
      <div class="mdl-card__supporting-text">Enjoy real-time piano performance by a neural network</div>

"Enjoy a"


demos/homepage/assets/style.css, line 260 at r1 (raw file):

#perf-rnn {
  background: url('https://storage.googleapis.com/learnjs-data/Piano/performance_rnn.gif') center / cover;

this is a weird spot for the image, can we keep it either in demos/images or in a separate directory under learnjs-data for static images? Piano was meant just for the Piano sounds (Piano/Salamander mirrors the tonejs directory structure, this now breaks it).


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

dsmilkov commented Oct 5, 2017

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


demos/homepage/index.md, line 125 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

"Enjoy a"

Done.


demos/homepage/assets/style.css, line 260 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this is a weird spot for the image, can we keep it either in demos/images or in a separate directory under learnjs-data for static images? Piano was meant just for the Piano sounds (Piano/Salamander mirrors the tonejs directory structure, this now breaks it).

It's 5 mb - moved to learnjs-data/images


Comments from Reviewable

@dsmilkov dsmilkov merged commit ffdc100 into master Oct 5, 2017
@dsmilkov dsmilkov deleted the magenta branch October 5, 2017 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants