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

implement the logic of current interval number #3561

Merged
merged 5 commits into from
Jan 13, 2024
Merged

implement the logic of current interval number #3561

merged 5 commits into from
Jan 13, 2024

Conversation

Aman1919
Copy link
Contributor

issue #2209

block is working in the interval block and notes blocks . tried in the examples given in the issue .

after

current.interval.mp4

@pikurasa pikurasa requested a review from walterbender January 12, 2024 00:40
@pikurasa
Copy link
Collaborator

current.interval.mp4

That output looks good to me. I'm recommend @walterbender to review the code.

@walterbender
Copy link
Member

Could you please add NOTESTEP to the list of constants exported in musicutils.js ?

@Aman1919
Copy link
Contributor Author

Could you please add NOTESTEP to the list of constants exported in musicutils.js ?

added notestep in musicutils.js

@walterbender
Copy link
Member

Looks good. But question for @pikurasa : right now, this block is "hidden". I presume we can expose it as an advanced block?

@pikurasa
Copy link
Collaborator

Looks good. But question for @pikurasa : right now, this block is "hidden". I presume we can expose it as an advanced block?

"Interval number" block is hidden?

@walterbender
Copy link
Member

Yes. Hidden on the palette by default...

@pikurasa
Copy link
Collaborator

Yes. Hidden on the palette by default...

Then, yes, we should expose it in our advanced mode.

@Aman1919
Copy link
Contributor Author

@walterbender just curious advance block means that block does not appear in the left menu bar . we only access the block by searching ? And every thing seems to be fine so can u merge this .

@walterbender
Copy link
Member

The block was marked as "hidden" since it was not working as Devin had wanted. Now that it is working, it should not be hidden so it appears in the palette.
See https://github.com/sugarlabs/musicblocks/blob/master/js/blocks/IntervalsBlocks.js#L274

Advanced vs Beginner is a separate flag. But this should be advanced, not beginner.
See https://github.com/sugarlabs/musicblocks/blob/master/js/blocks/IntervalsBlocks.js#L273

@Aman1919
Copy link
Contributor Author

Aman1919 commented Jan 13, 2024

The block was marked as "hidden" since it was not working as Devin had wanted. Now that it is working, it should not be hidden so it appears in the palette.
See https://github.com/sugarlabs/musicblocks/blob/master/js/blocks/IntervalsBlocks.js#L274

Advanced vs Beginner is a separate flag. But this should be advanced, not beginner.
See https://github.com/sugarlabs/musicblocks/blob/master/js/blocks/IntervalsBlocks.js#L273

Got it . Should I make changes for beginnerblock to advance block and hidden to false ?

@walterbender
Copy link
Member

Yes. And you can just delete the hidden line since false is the default.

@Aman1919
Copy link
Contributor Author

Yes. And you can just delete the hidden line since false is the default.

did the changes

@walterbender walterbender merged commit 96d68d1 into sugarlabs:master Jan 13, 2024
3 checks passed
@Aman1919 Aman1919 deleted the nb branch January 13, 2024 16:29
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.

3 participants