fix:resolves grid position on hamburger opening(#3914)

sugarlabs / musicblocks

Music Blocks -- A musical microworld

https://musicblocks.sugarlabs.org/

GNU Affero General Public License v3.0

558 stars 757 forks source link

fix:resolves grid position on hamburger opening(#3914) #3924

Closed BeNikk closed 3 months ago

BeNikk commented 3 months ago

Description

This PR fixes #3914

Changes Made

-Added positioning of grids according to toggle of the hamburger menu(aux-menu) -Changing the grid position by half of other elements for sync

Testing Performed

Tested on -Chrome
...

Checklist

[x] I have tested these changes locally and they work as expected.
[ ] I have added/updated tests that prove the effectiveness of these changes.
[ ] I have updated the documentation to reflect these changes, if applicable.
[ ] I have followed the project's coding style guidelines.
[ ] I have addressed the code review feedback from the previous submission, if applicable.

Additional Notes for Reviewers

Adding media for showing the fix- Screencast from 27-06-24 01:53:49 PM IST.webm

walterbender commented 3 months ago

This looks good. A couple of changes: please remove the console log message and please add spaces around operators, e.g., +5 ==> + 5

I wonder, though, if maybe we can just call resize() instead of doing all of this work here. Maybe you could experiment?

BeNikk commented 3 months ago

Done with the suggested changes, I tried using resize function before as well, but i guess toggling aux menu is not directly changing canvas height, so it does not work (if i am using the function it correctly).

walterbender commented 3 months ago

You are correct re resize.

But alas, at least with FF, I cannot get your code to work properly. The grid position is way off.

Screenshot from 2024-06-27 12-43-17

BeNikk commented 3 months ago

I tested on FF rn, it seems to work on my machine(vid attached). However, I am happy to implement any other solution you propose to handle this, but afaik, the solution seems correct. The only issue is when I am opening the aux for the first time, the shift of Mr.mouse is not aligned with the grid. after that it works fine.

Screencast from 27-06-24 11:25:31 PM IST.webm

walterbender commented 3 months ago

I will have to dig into your solution to see what is wrong.

BeNikk commented 3 months ago

Oh okay! Hope it's not too troublesome for you.

On Thu, Jun 27, 2024, 11:56 PM Walter Bender @.***> wrote:

I will have to dig into your solution to see what is wrong.

— Reply to this email directly, view it on GitHub https://github.com/sugarlabs/musicblocks/pull/3924#issuecomment-2195417507, or unsubscribe https://github.com/notifications/unsubscribe-auth/BEZGFJD3VPZJ6OXZ3IYJMWTZJRKMFAVCNFSM6AAAAABJ7OIO4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJVGQYTONJQG4 . You are receiving this because you authored the thread.Message ID: @.***>

walterbender commented 3 months ago

Please test the changes I made to your MR. I think it should work now.

BeNikk commented 3 months ago

Yes, its working perfectly now!

BeNikk commented 3 months ago

Thank you!