search

processing / p5.js-web-editor

The p5.js Editor is a website for creating p5.js sketches, with a focus on making coding accessible and inclusive for artists, designers, educators, beginners, and anyone else! You can create, share, or remix p5.js sketches without needing to download or configure anything.
https://editor.p5js.org
GNU Lesser General Public License v2.1
1.38k stars 1.32k forks source link

Enhance mobile keyboard shortcut responsiveness #2817

Closed sdivyanshu90 closed 6 months ago

sdivyanshu90 commented 9 months ago

Issue:

image

Fixes: Enhanced keyboard shortcut responsiveness on mobile devices.

Changes:

After the changes:

image

I have verified that this pull request:

lindapaiste commented 9 months ago

This will make it full-width on desktop, right? We don’t want that. How about we keep the existing width property but add a max-width of 100%.

sdivyanshu90 commented 9 months ago

Actually, no, it's not making it full-width on the desktop. Please refer to the screenshot below for clarification.

image

By the way, Happy New Year!

lindapaiste commented 9 months ago

Ah right I see. The .keyboard-shortcuts class is used for the contents of the modal rather than for the modal itself.

Now I'm playing in Chrome dev tools and toggling your change on and off. Two notes:

  1. With the width: 100% the scrollbar moves all the way to the edge of the modal because the margin-right is ignored. Maybe that's a good thing? But I'm always hesitant to change design because I'm not a designer. We could keep the scrollbar margin by subtracting it from the width. width: calc(100% - #{20 / $base-font-size}rem);

  2. The outer overlay doesn't have any defined width. The overlay width is based on the width of the contents. So when we change the contents to have a width of 100% that's the same (on desktop) as width: auto. Basically the width is based on the longest line of text. So there is a slight change where the width of the outer overlay used to be 472px but it's now 450.75px. Not very noticeable but it did change.

What I initially suggested was to keep the width as-is but use a max-width. Now that I am looking in depth I see that this actually looks very weird if the screen width is below 650px, where the outer overlay becomes full-width, but greater than 450px. In that size range the scrollbar won't be all the way on the right. Screenshot 2024-01-01 14 03 27

I'm going to ponder this a bit and see what I can think of. We could definitely use media queries, but there may be something cleaner.

sdivyanshu90 commented 9 months ago

Hi @lindapaiste, thank you for the review. Could you please take another look at the changes? I updated the width from width: 100%; to width: calc(100% - #{20 / $base-font-size}rem);.

image
sdivyanshu90 commented 9 months ago

Hey @raclim and @lindapaiste, can you please review the pull request?

sdivyanshu90 commented 8 months ago

Hey @raclim and @lindapaiste, can you please review the pull request?

raclim commented 6 months ago

I'm really sorry about this, but I realized I completely missed this PR and just merged in https://github.com/processing/p5.js-web-editor/pull/2932 from https://github.com/processing/p5.js-web-editor/issues/2866, which also addresses this issue (but does seem to find a solution for the weird screen width in the 450px-650px range!) 😅 I'm really sorry again about this, this was definitely on me to be more organized with reviewing these issues and PRs.

sdivyanshu90 commented 6 months ago

No problem at all! I understand, and thanks for letting me know. It happens, and I appreciate your efforts in addressing the issue. Let's continue moving forward together.