Mobile overlay fix: works on smaller devices #2346

dewanshDT · 2023-08-04T06:03:04Z

This is a part of mobile responsive project

Changes:
It changes the _overlays.scss file

Before:

After:

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.
is descriptively named and links to an issue number, i.e. Fixes #123

lindapaiste · 2023-08-04T15:03:33Z

It looks like there’s a vertical scroll bar in the “after” screenshot? So there’s a container around that content which needs to have a change to its height. Probably specific to the settings/preferences and not in this overlay.scss file. I know that I said to make a PR with just the overlay.scss changes so we could fix that later if you want or you could fix it now as part of this PR.

lindapaiste · 2023-08-04T15:23:07Z

client/styles/components/_overlay.scss

  position: relative;
  padding-bottom: #{25 / $base-font-size}rem;
+
+  @media (max-width: 650px) {


My first instinct was that we should centralize these breakpoints into SCSS and JS variables. Then I thought about it more and we don't necessarily need to use the same breakpoints for every component. The point at which the overlay no longer fits might be different than the point where the nav bar doesn't fit, for example. But if we want to add breakpoints to elements which are rendered as children of an overlay (like the preferences) then those would probably need to use the same breakpoints as the overlay. Curious your thoughts on this.

yeah I agree the breakpoints does not need to be same when they are completely unrelated like the navbar and the overlay

lindapaiste · 2023-08-04T16:42:59Z

There are some specific issues with specific modals. I DON'T WANT YOU TO FIX THESE. I'm just looking things over to see where we are at. We've always said that the collection lists, etc. would be something that you worked on last only if you had time since your main focus in on the IDE. Maybe we open some issues and let other contributors handle these?

Components rendered inside <Overlay/>:

AddToCollectionList (list of collections) - needs a max-width, move searchbar below header.
AddToCollectionSketchList (list of sketches) - needs a max-width, move searchbar below header.
Preferences - need to remove the explicit height on the container.
About - Content needs rearranging. Some of the changes in added styled component to About.jsx #2342 may help.
Feedback -- this is in the code but not actually in the app 🙃
ShareModal
KeyboardShortcutModal - almost good but we need a max-width on the container to force long lines to wrap.
ErrorModal
CollectionCreate - form fields need a max width.

lindapaiste

The purpose of this PR is to make overlays fill the screen on mobile devices and you've mostly done that. The one modal-specific issue which I think is worth addressing now is the height of the "fixed height" overlay, only because that is declared in this same _overlay.scss file. Our intention is that overlays should fill the whole screen on small devices so I think that should apply here too.

p5.js-web-editor/client/styles/components/_overlay.scss

Lines 50 to 53 in 31e64d5

    
           /* Fixed height overlay */ 
        
           .overlay--is-fixed-height .overlay__body { 
        
             height: 80vh; 
        
           }

We discussed the possibility of writing media queries "mobile first" with the mobile as the default and a min-width media query for the desktop styles. That might be good here because the height: 80vh; only applies on .overlay--is-fixed-height if the min-width query is met. If we write it desktop-first then we need two styles: height: 80vh and an override to set it back to height: 100vh; on small screens. Whereas with mobile-first we only need one style, with a selector and a media query, to apply height: 80vh. But I'll leave that up to you.

into dewanshmobile/overlayscss

dewanshDT · 2023-08-07T03:20:08Z

It looks like there’s a vertical scroll bar in the “after” screenshot? So there’s a container around that content which needs to have a change to its height. Probably specific to the settings/preferences and not in this overlay.scss file. I know that I said to make a PR with just the overlay.scss changes so we could fix that later if you want or you could fix it now as part of this PR.

yeah I didn't change that due to the reason that these changes were already present in the mega PR that you made me merge in before I started working, I think that we should get that part of the PR merged in the repo, what are your suggestions on this?

dewanshDT · 2023-08-07T03:25:54Z

There are some specific issues with specific modals. I DON'T WANT YOU TO FIX THESE. I'm just looking things over to see where we are at. We've always said that the collection lists, etc. would be something that you worked on last only if you had time since your main focus in on the IDE. Maybe we open some issues and let other contributors handle these?

Components rendered inside <Overlay/>:

AddToCollectionList (list of collections) - needs a max-width, move searchbar below header.

AddToCollectionSketchList (list of sketches) - needs a max-width, move searchbar below header.

Preferences - need to remove the explicit height on the container.

About - Content needs rearranging. Some of the changes in added styled component to About.jsx #2342 may help.

Feedback -- this is in the code but not actually in the app 🙃

ShareModal

KeyboardShortcutModal - almost good but we need a max-width on the container to force long lines to wrap.

ErrorModal

CollectionCreate - form fields need a max width.

Yeah I agree, as we leave some of the issues for others to fix, we should mention them in the Issues section, also we can add the good first issue label in them

dewanshDT · 2023-08-07T03:28:32Z

The purpose of this PR is to make overlays fill the screen on mobile devices and you've mostly done that. The one modal-specific issue which I think is worth addressing now is the height of the "fixed height" overlay, only because that is declared in this same _overlay.scss file. Our intention is that overlays should fill the whole screen on small devices so I think that should apply here too.

p5.js-web-editor/client/styles/components/_overlay.scss

Lines 50 to 53 in 31e64d5

/* Fixed height overlay */

.overlay--is-fixed-height .overlay__body {

height: 80vh;

}

We discussed the possibility of writing media queries "mobile first" with the mobile as the default and a min-width media query for the desktop styles. That might be good here because the height: 80vh; only applies on .overlay--is-fixed-height if the min-width query is met. If we write it desktop-first then we need two styles: height: 80vh and an override to set it back to height: 100vh; on small screens. Whereas with mobile-first we only need one style, with a selector and a media query, to apply height: 80vh. But I'll leave that up to you.

I'll check it out, I've never saw this before

lindapaiste · 2023-08-11T19:04:46Z

@dewanshDT I'd like to get this PR merged. I think we can fix the fixed height ones by changing the current CSS to this. Can you check it out?

 /* Fixed height overlay */ 
 .overlay--is-fixed-height .overlay__body { 
  @media (min-width: 651px) {
     height: 80vh; 
   }
 }

dewanshDT · 2023-08-11T19:06:38Z

Yeah, just a minute

dewanshDT · 2023-08-11T19:25:55Z

check it out

lindapaiste · 2023-08-11T19:51:19Z

client/styles/components/_overlay.scss

+
+  @media (min-width: 770px) {
+    height: 80vh;
+  }


Now I'm playing with this and my head is exploding 🤯 but I think that height: 100vh; is actually all that we need? Because the max-height: 80%; will apply over 651px so that's the same as height: 80vh;

So I should remove the media query then?

Yeah I think so. I don't like to have code that doesn't do anything. The overlay looks good though!

lindapaiste

This is a major improvement to mobile usability!

overlays-overflow-fix

cb7afdb

dewanshDT requested review from lindapaiste and raclim August 4, 2023 06:03

lindapaiste reviewed Aug 4, 2023

View reviewed changes

lindapaiste requested changes Aug 4, 2023

View reviewed changes

raclim added the Area: Mobile label Aug 4, 2023

Merge branch 'develop' of https://github.com/processing/p5.js-web-editor

6230167

into dewanshmobile/overlayscss

dewanshDT and others added 3 commits August 11, 2023 23:45

overlay height

65f8ad7

Merge branch 'develop' into dewanshmobile/overlayscss

b58e7ca

fixed height overlay media query

20eadb0

dewanshDT requested a review from lindapaiste August 11, 2023 19:25

lindapaiste reviewed Aug 11, 2023

View reviewed changes

dewanshDT requested a review from lindapaiste August 11, 2023 19:59

lindapaiste approved these changes Aug 11, 2023

View reviewed changes

lindapaiste merged commit cade211 into processing:develop Aug 11, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile overlay fix: works on smaller devices #2346

Mobile overlay fix: works on smaller devices #2346

dewanshDT commented Aug 4, 2023 •

edited

lindapaiste commented Aug 4, 2023 •

edited

lindapaiste Aug 4, 2023

dewanshDT Aug 7, 2023

lindapaiste commented Aug 4, 2023

lindapaiste left a comment

dewanshDT commented Aug 7, 2023

dewanshDT commented Aug 7, 2023

dewanshDT commented Aug 7, 2023

lindapaiste commented Aug 11, 2023

dewanshDT commented Aug 11, 2023

dewanshDT commented Aug 11, 2023

lindapaiste Aug 11, 2023

dewanshDT Aug 11, 2023

lindapaiste Aug 11, 2023

dewanshDT Aug 11, 2023

lindapaiste left a comment

	/* Fixed height overlay */
	.overlay--is-fixed-height .overlay__body {
	height: 80vh;
	}

Mobile overlay fix: works on smaller devices #2346

Mobile overlay fix: works on smaller devices #2346

Conversation

dewanshDT commented Aug 4, 2023 • edited

lindapaiste commented Aug 4, 2023 • edited

lindapaiste Aug 4, 2023

Choose a reason for hiding this comment

dewanshDT Aug 7, 2023

Choose a reason for hiding this comment

lindapaiste commented Aug 4, 2023

lindapaiste left a comment

Choose a reason for hiding this comment

dewanshDT commented Aug 7, 2023

dewanshDT commented Aug 7, 2023

dewanshDT commented Aug 7, 2023

lindapaiste commented Aug 11, 2023

dewanshDT commented Aug 11, 2023

dewanshDT commented Aug 11, 2023

lindapaiste Aug 11, 2023

Choose a reason for hiding this comment

dewanshDT Aug 11, 2023

Choose a reason for hiding this comment

lindapaiste Aug 11, 2023

Choose a reason for hiding this comment

dewanshDT Aug 11, 2023

Choose a reason for hiding this comment

lindapaiste left a comment

Choose a reason for hiding this comment

dewanshDT commented Aug 4, 2023 •

edited

lindapaiste commented Aug 4, 2023 •

edited