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

Adding Play-Only-Mode to MusicBlocks #4320

Merged
merged 14 commits into from
Feb 24, 2025

Conversation

justin212407
Copy link
Member

I have made some changes regarding the play-only-mode features, basically now the palette goes away whenever the play-only-mode is triggered. This mode is triggered automatically when the screen size goes below 768px in width or 600px in height. These are the basic changes i have made till now. I'll be adding more features to this to make this a better working feature in the upcoming days after taking inputs on what should be added. For now it looks something like this.

Untitled.video.-.Made.with.Clipchamp.1.mp4

@walterbender @pikurasa I am still not sure on what features should be added to the dropdown menu. Please take a look at this till now and suggest if this looks good.

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
@walterbender
Copy link
Member

@pikurasa What do you think? Maybe we want a message explaining that we are in playback only mode and that to enjoy the whole MB experience, you need a larger display?

@pikurasa
Copy link
Collaborator

@pikurasa What do you think? Maybe we want a message explaining that we are in playback only mode and that to enjoy the whole MB experience, you need a larger display?

I agree.

Would this be the usual method, or a more persistent message?

Also, do we want all of the buttons at the bottom-right of the screen? Home could be helpful. Perhaps expand and collapse could be helpful. I doubt zoom in and zoom out will be helpful.

We may consider:

  • A persistent message at the bottom that says "Play only mode. For the full Music Blocks experience, please open with a larger display."
  • Only the home button at the bottom-right corner

Thoughts?

@justin212407
Copy link
Member Author

@pikurasa sure that would make sure that the user gets the best of the experience that MusicBlocks can provide and about the expand and collapse i don't think it makes much of a sense to add that for a playback mode feature since its mostly just related to listening how the music works.

@justin212407
Copy link
Member Author

@walterbender @pikurasa Other than this for the dropwdown menu that we talked about you know of changing auxiliary menu with the dropdown one, i guess we can use the same element that has been used for the select language toggle, this would ensure uniformity across the codebase. What are your thoughts on this?

@pikurasa
Copy link
Collaborator

i guess we can use the same element that has been used for the select language toggle, this would ensure uniformity across the codebase. What are your thoughts on this?

If I'm following your proposal correctly, yes, I think it would be good to use that.

Certainly, we don't want to create any new methods.

@justin212407
Copy link
Member Author

sure then i'll implement that and update this accordingly

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
@pikurasa
Copy link
Collaborator

I tested it just now, and, yes, the automatic relocating of the blocks is nice.

You are probably already aware of this, but the icons are hard to read (white on white):

Screenshot from 2025-02-10 15-19-58

@walterbender
Copy link
Member

I suspect that the recently landed changes to the scaling the toolbars should take care of the white-on-white problem. Maybe do a rebase?

@justin212407
Copy link
Member Author

sure i'll try to rebase and check whether the issue still persists if it does i'll try fixing it in this pr itself

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
@justin212407
Copy link
Member Author

I have removed the buttons for the play only mode that were present on the bottom of the screen on the right hand side the only thing left to work upon is the dropdown menu which i will be working upon soon.

@justin212407
Copy link
Member Author

@walterbender i am still encountering the issue where the auxiliary menu falls down below on the canvas as @pikurasa mentioned even after rebasing. Should i fix the same in this pr itself?
image

@justin212407
Copy link
Member Author

justin212407 commented Feb 12, 2025

Other than that i am also encountering another issue that is whenever the width of the device falls below 400px the icons present on the main menu are not visible. I'll also try to fix the same here as this also comes under the scope of play only mode as the resolution in this is less than 768px which is the cutoff width for the play-only-mode to be automatically activated.

Untitled.video.-.Made.with.Clipchamp.2.mp4

@justin212407
Copy link
Member Author

@walterbender I have made the changes to the issue that i have highlighted above where the main menu is not visible under the screen resolution having width below 400px. Please review whether there could be anymore changes that are needed to fixing this issue.

Untitled.video.-.Made.with.Clipchamp.3.mp4

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
@walterbender
Copy link
Member

It is very strange that you were still seeing the toolbar problem. I'll investigate.

@walterbender
Copy link
Member

With your current branch, play back only is triggering too soon.
Screenshot From 2025-02-13 14-59-47

@justin212407
Copy link
Member Author

i'll test it and update it to you accordingly

@justin212407
Copy link
Member Author

I tested it locally and it seems to work fine on my side. Could you please recheck it once again?

Untitled.video.-.Made.with.Clipchamp.4.mp4

@walterbender
Copy link
Member

Just tested again. This time with Chromium. As soon as my browser window < 770 pixels wide, playback-only-mode triggers.

@walterbender
Copy link
Member

Screenshot From 2025-02-14 09-17-53

@justin212407
Copy link
Member Author

justin212407 commented Feb 14, 2025

Yeah 768 pixels is what i have kept as the trigger point for the play only mode to be triggered as most tablets have the lowest screen width of 768 and that's where resolution for phones begin
Should I reduce it further?

@walterbender
Copy link
Member

So I guess it is working...
@pikurasa is 768 OK with your classroom needs?

@pikurasa
Copy link
Collaborator

So I guess it is working...
@pikurasa is 768 OK with your classroom needs?

I think that should be sufficient.

Some old laptops have a width of 1280, and 768 is well enough above that.

I think you are aware of this, but the toolbar is still a little funny, especially in the range where it is transitioning to play only.

Screenshot from 2025-02-14 12-50-02
Screenshot from 2025-02-14 12-49-52

I didn't try to figure out what the range is, but I could, if you need me to.

@justin212407
Copy link
Member Author

yes i do know about the toolbar i haven't fixed it yet since there will not be an advanced mode in play only mode and there will only be a dropdown menu for that as we discussed so this issue won't be arising anymore. I have just been a bit busy with some Mb-v4 stuff i will be updating and marking this pr as ready very soon

@walterbender
Copy link
Member

/me will try to reproduce the toolbar issue. I hadn't been able to.

@justin212407
Copy link
Member Author

@walterbender the issue still persists

image

@justin212407
Copy link
Member Author

but it won't exist in play only mode as the auxiliary menu won't be present

@justin212407
Copy link
Member Author

@walterbender @pikurasa another doubt i have is should we make horizontal scrolling enabled by default in play only mode as it would be a much more accessible feature to have on a smart phone or on small screens in general while viewing the project?
Or do you want to go for the button feature in the play only mode as well?

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
@justin212407
Copy link
Member Author

@pikurasa the range for the auxiliary menu breaking is a screen width of 1171 px whenever it falls below that it breaks

@pikurasa
Copy link
Collaborator

@justin212407 when this is ready again for review, please let us know.

If you have questions, please let us know.

In the meantime, it may be nice to create a table of the intended behavior at different screensizes. We can include it somewhere in the MB documentation to help ourselves with future maintenance (by understanding how everything's suppose to work).

Here's a sketch to get you started:

xx or lower xx to yy yy or above
disable aux menu .. full menus
enable horizontal scroll .. disable horizontal scroll

@justin212407
Copy link
Member Author

Sure @pikurasa I will raise another pr which defines all the functions taking place at different screen sizes

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
turtle-singer.test.js

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

Copy link

✅ All Jest tests passed! This PR is ready to merge.

@walterbender
Copy link
Member

@pikurasa @justin212407 should we merge this PR and raise a separate one for the other enhancements?

@justin212407
Copy link
Member Author

@walterbender just give me a bit more time it is producing some breaking changes recently let me fix that and then we can definitely go ahead and merge it and then i'll raise another pr for the enhancements

Copy link

✅ All Jest tests passed! This PR is ready to merge.

Signed-off-by: Justin Charles <charlesjustin2124@gmail.com>
Copy link

✅ All Jest tests passed! This PR is ready to merge.

@justin212407
Copy link
Member Author

@walterbender @pikurasa I have fixed all the breaking changes if you want you can go ahead and merge it and i will create another pr for the enhancements. Please review this one and let me know if you would like any more changes to this branch itself.

@justin212407 justin212407 marked this pull request as ready for review February 24, 2025 05:00
@walterbender
Copy link
Member

So you'll address the toolbar issues in a separate PR?

@justin212407
Copy link
Member Author

justin212407 commented Feb 24, 2025

which one are you talking about the one where the aux menu was breaking or creating the toolbar in the play only mode ?
If it is about creating the toolbar for the play only mode i can do that right here in this pr if that is what you suggest
I will be creating another one which addresses the breaking of aux menu below screen width of 1171 px the one i am talking about is this:

@walterbender the issue still persists

image

@walterbender
Copy link
Member

At least in Chrome, the toolbars are a mess.

Screenshot From 2025-02-24 08-29-01
Screenshot From 2025-02-24 08-29-09

@justin212407
Copy link
Member Author

so should I fix this in play only mode in this pr itself?

@walterbender
Copy link
Member

I am still not sure when the regression occurred. But I just wanted to confirm that you were aware of these issues. We can address them in a separate PR.

@walterbender walterbender merged commit 78789e6 into sugarlabs:master Feb 24, 2025
5 checks passed
@justin212407
Copy link
Member Author

sure i will be raising two more prs then one for the play-only-mode enhancements and the other for fixing the breaking of aux menu

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