-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added the necessary features for Play-Only-Mode #4516
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
Added the necessary features for Play-Only-Mode #4516
Conversation
Signed-off-by: Justin Charles <[email protected]>
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@walterbender please review this. |
|
@justin212407 Could you please update the |
|
Yes I'll update that accordingly @walterbender @pikurasa where would you want the documentation/Readme.md for the play-only-mode to be at specifically? |
|
The main README.md file is the right place to document this. |
Signed-off-by: Justin Charles <[email protected]>
|
✅ All Jest tests passed! This PR is ready to merge. |
…into play-only-mode-additions
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@walterbender I tested the same on Microsoft Edge too. This is how it looks: Untitled.video.-.Made.with.Clipchamp.7.mp4 |
…into play-only-mode-additions
|
✅ All Jest tests passed! This PR is ready to merge. |
|
I am not sure what is going on, but I cannot reproduce the behavior you are seeing in either Firefox or Chromium. |
|
Toolbaar does not work below 400px, otherwise works fine Screencast.from.2025-03-15.08-17-09.webmTested on Firefox and Brave |
@walterbender Did you go full screen? Play-only-mode only works in full screen. Also the screen width should definitely be below 768px it will work only then. |
@Karan-Palan might have gotten overlooked thanks for pointing it out I'll fix that and push the changes to this pr once walter can reproduce this locally. |
|
@walterbender since you are unable to reproduce the behavior should I close this pr and open a new one with with same changes? |
|
I don't know. Maybe @pikurasa can test? |
|
Sure we can definitely do that. I also asked @apsinghdev to check this out in the last meet if it's possible |
|
I set up the following for testing on a phone: https://sandbox.musicblocks.net/test/pikurasa/ Currently it's on "HEAD is now at c8a1363 Merge branch 'master' of https://github.com/justin212407/musicblocks into play-only-mode-additions" (and it requires manual updates). |
|
Here are the results of testing via Chromium on a Pixel 9 mobile phone: Specifically, the improvements would be:
So those are the results of my testing on a Pixel 9 mobile phone. Based on this, the improvement areas are:
|
play_mode.mp4@justin212407 I am able to see the changes you mentioned but the UI is not responsive. What if we tackle the issue of responsiveness first and then circle back to it? (As this issue depends on the UI). Also, I agree with the improvements suggested by @pikurasa. Tackling these will lead to a really good outcome. Probably, you'll need to prioritize the improvements. |
|
@apsinghdev go into full screen mode then you won't face the responsiveness issue |
yes i will be trying to look into this as for the fullscreen mode I will probably have to dig deeper into how dpr works. For now i will implement points 2,3 and 4 as they are an urgent fix. |
Signed-off-by: Justin Charles <[email protected]>
|
✅ All Jest tests passed! This PR is ready to merge. |
Signed-off-by: Justin Charles <[email protected]>
|
✅ All Jest tests passed! This PR is ready to merge. |
Signed-off-by: Justin Charles <[email protected]>
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@justin212407 where are things at with this? I see a few new commits. Is it ready to test again? |
|
@pikurasa i did make some changes but as we talked about in the meet, we need to find out the root cause as to why this change has happened so as discussed in the meeting. I am checking out the prs which made changes to the index file. So that we can fix this change between FullScreen Mode and Normal Mode having the same changes reflect in both so that it is a permanent fix and not just a temporary one. |
If you want to write up a blog about your investigation, that could be nice. It may even help you work through the problem. |
|
@pikurasa Sure i will definitely do that |
…o play-only-mode-additions
|
✅ All Jest tests passed! This PR is ready to merge. |
…o play-only-mode-additions
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@pikurasa is this ready to merge? |
Signed-off-by: Justin Charles <[email protected]>
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@walterbender @pikurasa this is almost done have brought this feature to normal screen too now the only thing left is having horizontal scroll |
|
This pull request has been open for more than 60 days without any activity. It will be closed in 3 days unless the |
|
Closed pull request due to inactivity for more than 63 days. |



Description:
This PR is in continuation to #4320. It adds the features to Play-Only-Mode that has previously been discussed on.
Changes Made:
This PR ensures that the only elements being rendered upon in play-only-mode are:
This also ensures that the horizontal scrolling feature is activated as default in play-only-mode.
Screenshots:
Untitled.video.-.Made.with.Clipchamp.6.mp4
Checklist: