Enhance mobile keyboard shortcut responsiveness

[sdivyanshu90]

Issue:

Fixes:
Enhanced keyboard shortcut responsiveness on mobile devices.

Changes:

• Replaces width: #{450 / $base-font-size}rem with width: 100%;

After the changes:

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.

[lindapaiste]

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]

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

By the way, Happy New Year!

[lindapaiste]

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.

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]

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);.

[sdivyanshu90]

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

[sdivyanshu90]

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

[raclim]

I'm really sorry about this, but I realized I completely missed this PR and just merged in #2932 from #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]

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.