search

techvalidate / pano

MIT License
0 stars 0 forks source link

Modal fixes for CX #48

Closed tjdo closed 7 years ago

tjdo commented 7 years ago

Modals: Allows scrolling, corrects vertical centering issues Fixes issues on dropzone where the upload message is not hidden during upload

Fixes techvalidate/cx/#2412 Fixes techvalidate/cx/#2382

mmlindeboom commented 7 years ago

@tjdo Just one thing that's breaking Engage modals. Other than that it looks good. If you can move that max-height to CX rather than Engage, I'll go ahead and merge this.

tjdo commented 7 years ago

The max-height is to allow the modals to scroll. Otherwise they overflow the viewport. I'm kind of surprised it's not scrolling in Engage.

tjdo commented 7 years ago

Question: Engage is using the 2.1 Pano branch, right?

mmlindeboom commented 7 years ago

@tjdo yes, I'm actually unclear about the reason for the point differences.

mmlindeboom commented 7 years ago

@tjdo They definitely scroll, but it blocks content -- even on my large screen -- which is definitely not ideal. When are modals overflowing the viewport?

jonwolfe commented 7 years ago

question: will all modals now be 90% of the viewport height even if they have very little content inside?

On Thu, Sep 14, 2017 at 11:31 AM Matt Lindeboom notifications@github.com wrote:

Merged #48 https://github.com/techvalidate/pano/pull/48.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/techvalidate/pano/pull/48#event-1249782054, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUFBiW7mt-33bFsR9bNf1plNHOK-Loks5siXEBgaJpZM4PXzzc .

-- Jonathon Wolfe Engineering Director SurveyMonkey 510-517-4344

tjdo commented 7 years ago

@jonwolfe my comment was a bit of a misnomer. It's actually max-height: 90%. So to answer your question, no. However, as a simple, initial fix for modals on mobile I have set to those to fixed height/width of 90%. We haven't really done anything to address mobile modals, so this was a simple add to avoid issues for anyone using mobile and seeing those modals.

jonwolfe commented 7 years ago

Gotcha. On mobile, we should probably use css to make modals full screen

On Thu, Sep 14, 2017 at 12:07 PM TJ Downes notifications@github.com wrote:

@jonwolfe https://github.com/jonwolfe my comment was a bit of a misnomer. It's actually max-height: 90%. So to answer your question, no. However, as a simple, initial fix for modals on mobile I have set to those to fix height/width of 90%. We haven't really done anything to address mobile modals, so this was a simple add to avoid issues for anyone using mobile and seeing those modals.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/techvalidate/pano/pull/48#issuecomment-329579773, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUFIA9x2-pGxdaJWOyKUA1kKS8WDADks5siXlngaJpZM4PXzzc .