-
Notifications
You must be signed in to change notification settings - Fork 481
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
Music Lab: initial prototype work #47808
Conversation
Needs proper variable handling still, though it does manage to loop sound placement.
It only checks against a hardcoded variable called measure.
No need to show a horizontal scrollbar.
…layout Music prototype: fix SCSS layout
…tructions-scrollbars Music prototype: only show vertical scrollbar for instructions
A second (and now simpler) attempt at reducing the blockly text to 14px.
…ockly-text-3 Music prototype: shrink blockly text
We don't support localization for now.
…cale-support Music prototype: Update locale support
…renderers Music prototype: separate blockly renderers
…tra-styling Music prototype: remove extra styling
It was just removed for the convenience of early development.
…eacher-panel Music prototype: restore teacher panel
The user can click the small instructions image to see a big version, and click that to go back to the small one. Also, the instructions text scrollbar is only shown when necessary.
…uctions-image Music prototype: big instructions image
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.
A few random questions but looks good to me! I'm new to blockly so it would be good to get an approval from someone who knows more about it :)
import PropTypes from 'prop-types'; | ||
import React from 'react'; | ||
|
||
export default class Instructions extends React.Component { |
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.
I'd be interested to hear if you all are hoping to replace instructions with a pattern like this or if this is intended to be only for music lab
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.
This is just a temporary component that can be replaced by a real instructions component once we integrate with our level system.
@@ -0,0 +1,15 @@ | |||
import GoogleBlockly from 'blockly/core'; | |||
|
|||
export default class CdoPathObject extends GoogleBlockly.zelos.PathObject { |
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.
for my learning: what is zelos
?
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.
Learned this recently, they're different blockly renderers. I think their docs are kind of broken at the moment so I can't see the main zelos renderer page. No idea why they're using greek names 🤦
var hooks = {}; | ||
|
||
class UnconnectedMusicView extends React.Component { | ||
static propTypes = { |
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.
is this a class component because of the need for redux?
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.
Yeah, this just started out as a class component but with the need for Redux and the fact that it currently houses a lot of the blockly logic, it made sense to keep it a class. As we move stuff out of here it would be ideal to convert this (and the other views) into a functional component.
@@ -187,6 +189,12 @@ function initializeBlocklyWrapper(blocklyInstance) { | |||
CdoRenderer, | |||
true /* opt_allowOverrides */ | |||
); | |||
blocklyWrapper.blockly_.registry.register( |
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.
Preeety sure this shouldn't affect anything given we explicitly set the renderer elsewhere in this file, but maybe worth a quick check that this doesn't affect rendering of any of our google blockly labs (eg, poetry)?
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.
Glad you called this out, as it was a specific concern. I've been manually checking https://studio.code.org/flappy/10 to make sure we don't change behavior in an existing Google Blockly level that uses the other renderer.
@@ -1,6 +1,6 @@ | |||
.extra-links | |||
%h4 | |||
Extra Links: | |||
%span{onclick: 'return extraLinksDismissClick()'} Extra Links: |
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.
What is this change doing?
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.
Hah, this was a little convenience that I'll remove from this PR. It allowed the viewer to dismiss the admin menu in the bottom-left corner by clicking on its "Extra links:" text. :)
…ew-fixes Music prototype: code review fixes
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.
I skimmed a lot of the React and tried to focus on the pieces that touch other stuff, but I think that part looks good! Looks like Molly took a closer look at some of the React, so I think you have some decent coverage of most of what's here. Exciting stuff!
@@ -0,0 +1,118 @@ | |||
import WebAudio from './soundSub'; |
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.
One other q -- is all of this new relative to our existing Sound.js file? Or could be refactored/combined at some point? Or do we intentionally want to keep this separate?
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.
This is all new. I looked at the existing code but found it a bit confusing, especially since it supported both Web Audio and HTML Audio in the one file. (A great candidate for a factory that provides one or the other implementation, as it happens.). I had this code from previous hobby projects and it fit the needs well here. Certainly open to a unification, and would love if we had a common library that served all needs, but that feels like a nontrivial effort by itself.
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.
I also find the existing code supporting both audio types in one file pretty confusing, wonder if we could split it out at some point.
Music prototype: update media URL
This is the initial prototype of Music Lab.
This standalone app is designed to allow the creation of music with code.
i
: show/hide instructionst
: toggle timeline location, above/below the blocksd
: sample block mode = play block + dropdownv
: sample block mode = valuesp
: sample block mode = play block + valuesspace
: play/stop song1
,2
,3
,4
,5
,6
: trigger buttonsSome changes of interest: