MusicKeyboard: BugFixes, ES6 port, linting.

[ricknjacky]

Issue references:- #2767, #2630, #2609 #2629

In this PR, I have:-

• Ported widgets/musickeyboard.js file to ES6 class syntax.
• Fixed bug described below.
• Took care of linting and prettifying the file.

Bug

MusicKeyboard's black keys don't display text ( i.e. which key they represent respectively)

Obviation of bug post fixes and also the test after ES6 PORT.

MusicKeyboard_After.mp4

[meganindya]

Merging default branch into feature branches is not a good practice. If it is essential, perform a rebase.

[meganindya]

• Text on keys are to be visible only for pitches inside the widget's block.
• Add a margin/padding at the bottom of the text on the black keys.
• Any modification action on the column labels (duration) at the bottom of the matrix must close the pie menu.
• Any modifications on the pitch labels on the left of the matrix should reflect immediately in the widget's blocks. That includes adding new pitch/hertz blocks.
• The matrix seems to behave oddly after changing a pitch, closing the widget, and reopening it. Check pls.

[meganindya]

I performed an eslint --fix on the entire repo which affected 77 files. I've fixed the conflicts on this branch and fixed linting problems in WidgetBlocks.js. Pls hard reset to 2efcabe before continuing.

[ricknjacky]

• Text on keys are to be visible only for pitches inside the widget's block.

Didn't get you this time. Can you please come again?

• Add a margin/padding at the bottom of the text on the black keys.

Done!

• Any modification action on the column labels (duration) at the bottom of the matrix must close the pie menu.
• Any modifications on the pitch labels on the left of the matrix should reflect immediately in the widget's blocks. That includes adding new pitch/hertz blocks.
• The matrix seems to behave oddly after changing a pitch, closing the widget, and reopening it. Check pls.

I checked for both of these behaviors with My current local and master(musicblocks website), they are the same. Please tell me if I am missing something here. Thanks

[meganindya]

• Text on keys are to be visible only for pitches inside the widget's block.

The widget block contains a do4 (C4), re4 (D4), mi4 (E4), and sol4 (G4) and only these 4 are labelled on the keyboard.

[meganindya]

• Any modification action on the column labels (duration) at the bottom of the matrix must close the pie menu.
• Any modifications on the pitch labels on the left of the matrix should reflect immediately in the widget's blocks. That includes adding new pitch/hertz blocks.
• The matrix seems to behave oddly after changing a pitch, closing the widget, and reopening it. Check pls.

I checked for both of these behaviors with My current local and master(musicblocks website), they are the same. Please tell me if I am missing something here. Thanks

I know that. Somehow regressions have been introduced. This isn't meant to behave in this fashion. Gotta fix them.

[ricknjacky]

• Text on keys are to be visible only for pitches inside the widget's block.

The widget block contains a do4 (C4), re4 (D4), mi4 (E4), and sol4 (G4) and only these 4 are labelled on the keyboard.

So, text on musickeyboard's keys should be visible for the rest of keys present right?

[meganindya]

The widget block contains a do4 (C4), re4 (D4), mi4 (E4), and sol4 (G4) and only these 4 are labelled on the keyboard.

This is the expected behavior. Label only for those blocks in the widget's clamp.

[ricknjacky]

The widget block contains a do4 (C4), re4 (D4), mi4 (E4), and sol4 (G4) and only these 4 are labelled on the keyboard.

This is the expected behavior. Label only for those blocks in the widget's clamp.

Thanks for the clarification.
I have a proposition though, do4 (C4), re4 (D4), mi4 (E4), and sol4 (G4) are the pitches we've added using blocks and they should be represented, correct. But wouldn't it be a nice feature to have text for all keys present in the keyboard? That way it'll be more easy for beginners. Just a suggestion though.

Moreover, if we can play a particular key {black keys in this case}, it's a nice feature to have text representing which key it is designated to.

Your thoughts on this?

[meganindya]

Well then, perhaps could use different colors. Something lighter for all placeholder labels and and ones whose blocks are present, highlighted. Although, the idea is that only the pitches present in the widget's clamp would be required to play the music the user wants.

[ricknjacky]

Well then, perhaps could use different colors. Something lighter for all placeholder labels and and ones whose blocks are present, highlighted. Although, the idea is that only the pitches present in the widget's clamp would be required to play the music the user wants.

any suggestions on which color i should go ahead with?

[meganindya]

Try to do the rest first. Perhaps we can discuss this after.

[ricknjacky]

Try to do the rest first. Perhaps we can discuss this after.

Sure

About the modifications that you have referenced, They work smoothly for me, have a look at the test-video below for your perusal

musickeyboard.mp4

Notice at the bottom right of how do 4 is changed to ti 7 immediately as I make some modifications.

[meganindya]

Add pitch/hertz doesn't work as expected. When you add a note, force a reload.

New created blocks are being inserted into the widget clamp block when you add another. e.g. you try to add a hertz block, it doesn't show. You then add a pitch, the pitch isn't inserted, but the previous hertz block now gets added.

[ricknjacky]

Add pitch/hertz doesn't work as expected. When you add a note, force a reload.

New created blocks are being inserted into the widget clamp block when you add another. e.g. you try to add a hertz block, it doesn't show. You then add a pitch, the pitch isn't inserted, but the previous hertz block now gets added.

Right, This is a regression and it's prevalent in all widgets where in we can add new notes be it pitch, hertz or drum. I tried testing it with Phrasemaker widget and behavior was erratic there as well. I am not quite sure where this bug is, will dig and try to solve it in a different PR.

As is, this PR focused on porting, Linting and bug that I have described above.

[meganindya]

Choose a contrasting shade of grey from platformColor for the ones not in the block.

[ricknjacky]

Choose a contrasting shade of grey from platformColor for the ones not in the block.

Um, wouldn't that scuttle the UI of Keyboard? IRL, white and black-keys is how most or probably every music keyboard looks like. Do we really want mix of colors: ones that are in and that are not in "block"?

[meganindya]

@walterbender what do you think about labels for pitches not inside the widget clamp? Right now, those aren't labeled.

[walterbender]

Regarding labelling, I think we should only label the keys designated by the user in the widget clamp. The other keys are there to provide context/structure.

As far as the labels themselves, where possible, they should be letter names since the keys of a piano are unambiguous. But a hertz block may or may not map onto a letter name, depending on the temperament, so I think that hertz should be a number and treated separately as per the current design.

[walterbender]

I would rather have the "rest" be under the keyboard, like a space key on a typewriter, but that is outside of the scope of this PR.

[ricknjacky]

@walterbender so any issues with this PR ??
Should I modify something here?

[ricknjacky]

ps:- Firstly, I was planning on working on the bug(regression) @meganindya has rightly pointed out when we add new notes/pitches/drums (in phrasemaker widget). I am working on finding the cause of the regression, I think I'm close, hopefully will raise a PR addressing it soon.

Then, eventually foray into working on the UI enhancements as suggested above.

[walterbender]

I will try to review this weekend.

[walterbender]

Hmm. The labels are still appearing on the black keys.

[ricknjacky]

Hmm. The labels are still appearing on the black keys.

Apologies if I have misinterpreted this all along, when I started working on this widget, I was under the impression that Black Keys not displaying the text that they're supposed to(irrespective of whether or not they're part of blocks we've added{clamps}) , is a bug ;-; Any suggestions on changes I should make?

[walterbender]

There had been a bug where the black keys were being mislabeled. But I thought that was fixed.
The only keys that should be labeled -- black or white -- are the ones with corresponding pitch blocks in the widget clamp.

[ricknjacky]

There had been a bug where the black keys were being mislabeled. But I thought that was fixed.
The only keys that should be labeled -- black or white -- are the ones with corresponding pitch blocks in the widget clamp.

Done!

[walterbender]

It is really impossible to review this since there are so many unrelated changes scattered throughout your commits. It is good practice to confine each commit to one theme: (1) adding doc strings; (2) lint formatting changes; (3) bug fix. As it is I don't know where to look for your bug fix.

[ricknjacky]

It is really impossible to review this since there are so many unrelated changes scattered throughout your commits. It is good practice to confine each commit to one theme: (1) adding doc strings; (2) lint formatting changes; (3) bug fix. As it is I don't know where to look for your bug fix.

Absolutely sorry for causing the trouble. 😔

Also, bug was due to me changing the way we declare octave.

Initially, I had removed the let octave declaration, declared it as const octave inside the if block and then, it was confined to that,

Also, I had re-declared it again on line 2057 for using it in this.layout I had misinterpreted the behavior all together, won't repeat the mistake again 🙏

Apologies again, for the trouble caused. Doing a quick rebase.

[walterbender]

Could you please submit a new PR with three separate commits, one each for your bug fix, your ES6 port, and your linting.