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

piano project #36

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

piano project #36

wants to merge 2 commits into from

Conversation

gargonmi
Copy link
Collaborator

@gargonmi gargonmi commented Dec 1, 2018

No description provided.

</div>
<div id='playedNote' ></div>

<div id='imslp'>
Copy link
Contributor

Choose a reason for hiding this comment

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

igual usaría un ID un poco más intuitivo y que represente más claramente la intención

Copy link
Contributor

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Feedback

  • Proyecto super creativo! Has invertido mucho tiempo investigado el tema musical para poder aterrizar toda la aplicación y se nota 👍
  • La idea de añadir soporte MIDI fue increíble y de nuevo.. representa muy bien tu trabajo y el esfuerzo que dedicas a los proyectos :-)
  • Usaría un linter para evitar fallos tontos como no definir variables.
  • Por otro lado... reorganziaría un poco mejor el código para fuera más legible la ejecución y también para ahorrarle trabajo al interprete de JavaScript.

@@ -0,0 +1,264 @@
(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy bien ⭐️

@@ -0,0 +1,264 @@
(function () {

window.onload = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

No te hace falta esperar a la carga... Más info

(function () {

window.onload = function () {
renderPiano();
Copy link
Contributor

Choose a reason for hiding this comment

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

Puedes pasarle la función renderPiano directamente como argumento si usaras addEventListener o incluso podrías pasarle la ejecución tal y como esta ahora.

var piano = document.getElementById('piano');
var index = keyIndex;

notes = [
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ No te olvides de definir las variables

Copy link
Contributor

Choose a reason for hiding this comment

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

Tambien sacaría este bloque fuera... como un recurso externo


for(var tecla = 0 ; tecla < 12 ; tecla++) {

var key = document.createElement('div');
Copy link
Contributor

Choose a reason for hiding this comment

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

Sacaría esto en funciones más pequeñas y especializadas

input.value.onmidimessage = onMIDIMessage;
}

console.log('Got midi!', midi.inputs.values());
Copy link
Contributor

Choose a reason for hiding this comment

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

No debería estar en pro ;-)


//funcion para trasformar el numero de nota (midi) a una frecuencia

function midiNoteToFrequency (note) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy bien. Super claro! 🌟


// traer listado de trabajos
// nodos del dom
imslp = document.getElementById('imslp');
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ No te olvides de definir las variables!

// traer listado de trabajos
// nodos del dom
imslp = document.getElementById('imslp');
spinner = document.createElement('img');
Copy link
Contributor

Choose a reason for hiding this comment

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

Lo podrias dejar ya metido en el HTML... pero es cuestión de gustos :-)

})
}

function getWorks(partitura, posicion) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bien gestionada la asincronía con las promesas ^^

@gargonmi
Copy link
Collaborator Author

Muchas gracias Ulises.
Me pongo con ello para corregir lo que me indicas.

@UlisesGascon UlisesGascon mentioned this pull request Jan 15, 2019
46 tasks
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.

None yet

2 participants