implemented current interval block #3679
Conversation
|
@pikurasa could u review the changes ? |
I tested it just now via Two things I noticed:
I haven't yet tested keys other than C major. I did test chromatic, and some of the results are unexpected, but I suspect that has to do with the note names we are giving it not with the way we are measuring them, so I don't expect you to resolve that in this ticket (but I thought worth mentioning). |
@pikurasa ok . I have done changes . It is displaying now current.interval3.mp4 |
|
It works brilliantly, as far as I can tell. It works across different keys as well, which is great. Would you mind making the following (small) changes to the text (and capitalization) for consistency? "two octaves above" --> "Two perfect octaves above" "an octave below" --> "A perfect octave below" Then, it should be ready for code review by @walterbender |
|
@walterbender I have done the change . Could you please review the code ? |
js/turtleactions/IntervalsActions.js
Outdated
| if (index1 > index2) letterGap = NOTENAMES.length - letterGap; | ||
|
|
||
| let totalIntervals = this.GetIntervalNumber(turtle); | ||
| const numberToStringMap = ['an', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine'] |
There was a problem hiding this comment.
"an" is not going to work well with translation. Please change to "one".
Note that any string that is to be translated needs to be inside _(), e.g., _("two")
There was a problem hiding this comment.
@walterbender should i just do this _() to the return value ?
There was a problem hiding this comment.
Better to add it to each string. Otherwise they won't be added to the list of strings to translate
There was a problem hiding this comment.
I have done the changes
js/turtleactions/IntervalsActions.js
Outdated
|
|
||
| if (totalIntervals > 21) { | ||
| if (octave >=1) { | ||
| lastWord = ", " + _('plus') + " " + os + " " +plural; |
|
The interval shouldn't depend upon the order of the pitches in the note block:
|
Oh yes, I hadn't seen that behavior. Those should both be minor 3rd (Sol_4 to Mi_4). |
|
@walterbender did changes for this . I think before it going to above octave . fixed it . interval2.mp4 |
Issue #2209
currentinterval2.mp4