Change max-height of ModalContainer

[malorie16]

The previously hardcoded max-height of 700px was causing issues on mobile but changing it to 100% fixes it right up 🎉

Before

After

[michaeljaltamirano]

This fixes the iPhone5 resolution scenario, but then creates a sub-optimal experience on Desktop, taking up the whole viewport, which is not the intent of the DialogModal:

You may be able to wrap the styling in a media query in order to have it only apply to low-resolution devices. However, this seems to me like a violation of the distinction we try and maintain between DialogModal and ImmersiveModal: "Dialog modals shouldn't contain large content and should not scroll unless screen size dictates it. To display large amounts of content, use Immersive modal instead.". I think this example qualifies as large amounts of content.

I would try and see if you can wrap <UrgentTriageModal /> in an ImmersiveModal instead of a DialogModal. You will likely have to make a few changes if you proceed down this route (e.g. making sure the Report a Medical Emergency title persists in the transition as you scroll down). I would consult with stakeholders depending on the route you take--I can follow up later if you'd like more guidance but I have a meeting right now and wanted to get the comment to you 😬

[malorie16]

I'll take a look at the ImmersiveModal and may reach out on Monday with more questions!

[daigof]

I think you need 2 things from each type of modal:

• display scrolling content (Immersive)
• Force user to make an action (Dialog)

Immersive Modals always have a close a button and you can close clicking outside of it. So probably that was not intended for your UrgentTriageModal

I can think of 2 things to suggest:

• Test with immersiveModal and get an approval to go with it
• Since this is an edge case: test passing custom styles: These modals support the {...rest} props if you check the code it is passed to the Overlay component so you could pass a css prop with your own styles in your consumer app, you need to get to the ModalContainer via a css selector and add the mediaquery there (in your consumer app that is)

[malorie16]

I think you need 2 things from each type of modal:

• display scrolling content (Immersive)
• Force user to make an action (Dialog)

Immersive Modals always have a close a button and you can close clicking outside of it. So probably that was not intended for your UrgentTriageModal

I can think of 2 things to suggest:

• Test with immersiveModal and get an approval to go with it
• Since this is an edge case: test passing custom styles: These modals support the {...rest} props if you check the code it is passed to the Overlay component so you could pass a css prop with your own styles in your consumer app, you need to get to the ModalContainer via a css selector and add the mediaquery there (in your consumer app that is)

Thank you for this input Diego! On mobile, the ImmersiveModal takes up the full height of the viewport (-32px) and with some tweaking in the dev tools I wasn't able to get the desired changes needed so that it took up a similar height to that of the DialogModal. This tells me that a more substantial change may be needed to the ImmersiveModal which I don't think is worth the trouble for this bug. What I'm going to try next is writing a media query like Michael mentioned and/or passing custom styles like you mentioned so that I can try and override the max-height: 700px on the DialogModal.

This is what I got with the ImmersiveModal out of the box:

[malorie16]

@daigof and @michaeljaltamirano I didn't have any luck accessing the ModalContainer that lives within within the ImmersiveContainer from the consumer app so I added a media query here. Adding styling with a css prop (or even within the styled component) was only applied to the initial "How can we help?" modal but not the "Report an Emergency" modal and I noticed clicking the "Report an Emergency" button didn't trigger a re-render of the component although it's hooked into a store/mobx context (mobx magic? - still learning about this library). The media query here should just work but any feedback on a different approach would be much appreciated!

[michaeljaltamirano]

You can use MEDIA_QUERIES directly as in line 75. We typically develop "mobile first" which really means the base styles we set are for all views, and then we start adding exceptions starting at wider viewport widths:

- max-height: 700px;
+ max-height: 100%

+ ${MEDIA_QUERIES.smUp} {
+  max-height: 700px;
+ }

[michaeljaltamirano]

@daigof and @michaeljaltamirano I didn't have any luck accessing the ModalContainer that lives within within the ImmersiveContainer from the consumer app so I added a media query here. Adding styling with a css prop (or even within the styled component) was only applied to the initial "How can we help?" modal but not the "Report an Emergency" modal and I noticed clicking the "Report an Emergency" button didn't trigger a re-render of the component although it's hooked into a store/mobx context (mobx magic? - still learning about this library). The media query here should just work but any feedback on a different approach would be much appreciated!

I've opened a PR that (I think) shows how to switch the UrgentTriageModal functionality to using an ImmersiveModal. There is indeed some mobx magic so even if you decide to go the radiance-ui-only route I hope it is instructive 🙂 https://github.com/curology/PocketDerm/pull/12143

[michaeljaltamirano]

"Let's get this merged!"

As noted in my previous comment, I think there is an adequate user-land solution here, but I also think it's eminently reasonable to add this logic so that we prevent ourselves from overloading DialogModal for lower-end devices! Thank you for your keen eye and penchant for setting dev tools to iPhone5/SE 💪

If it isn't urgent, I think we can wait for Diego's second look since he has more exposure to these modals, but I think this solution works.

[daigof]

sorry I haven't have a chance to test this thoroughly, please implement Michael change but please try with with

 ${MEDIA_QUERIES.xsUp}

Check iphones dimensions reference:

• iPhone 5: 320px
• iPhone X: 375px
• xs breakpoint: 330px

This way you will be targeting/fixing iPhone5 issue without modifying the iPhone X experience which is the device most of our users use (in terms of width dimensions). Test and let me know if that solves your bug Ill test once this is ready. Using MEDIA_QUERIES.xsUp is the most harmless approach, I hope it fixes your issue

[michaeljaltamirano]

sorry I haven't have a chance to test this thoroughly, please implement Michael change but please try with with

 ${MEDIA_QUERIES.xsUp}

Check iphones dimensions reference:

• iPhone 5: 320px
• iPhone X: 375px
• xs breakpoint: 330px

This way you will be targeting/fixing iPhone5 issue without modifying the iPhone X experience which is the device most of our users use (in terms of width dimensions). Test and let me know if that solves your bug Ill test once this is ready. Using MEDIA_QUERIES.xsUp is the most harmless approach, I hope it fixes your issue

I don’t think this is correct. Here is a screenshot from my iPhone 12 Pro:

Even on this high-end device I have to scroll the close icon into the viewport. It does not stay in the viewport because of the 700px Max-height and all the content.

The real issue is that this content shouldn’t be in a dialog modal, it is too much content. Nevertheless, smUp should be used instead of xsUp. Changing max-height from 700px to 100% will not change the experience for even higher end mobile devices beyond making the close icon actually accessible without needing to scroll it into view.

[michaeljaltamirano]

I forgot we can add videos directly now and that I can take a screen recording of my phone 😅

IMG_9006.MP4

[daigof]

That's fine I thought we were targeting iPhone5 only, I ahvent had the chance to test, but the most restrictive breakpoint that works Im ok with it.