standardize modal sizes #2126

szaimen · 2021-07-20T13:55:16Z

Currently, there are many open points regarding modal sizes that each developer needs to decide and implement. This leads to unecessary code duplication, maintenance effort and inconsistent UI because each developer implements the sizing differently. Hence here is my proposal to standardize the modal sizes completely.

It also includes always using full screen modal on mobile which is the superior experience on mobile, imho.

Signed-off-by: szaimen [email protected]

skjnldsv · 2021-07-20T16:12:21Z

src/components/Modal/Modal.vue

 	.modal-container {
 		display: block;
-		overflow: hidden;
+		overflow: auto; // avoids unecessary hacks if the content should be bigger than the modal


Breaking change

yes, I know, hence I've added the breaking label to the PR. But it is necessary to make this work reliably.

If we're following SemVer then this would consitute a new major :)

Jep. Would it be 4.1.0 or 5.0.0 ?

Added milestone 5.0.0 :)

src/components/Modal/Modal.vue

szaimen · 2021-07-20T16:49:23Z

Thanks for the review @skjnldsv!
I've just reworked some small things and added two more sizes which could potentially be useful.

szaimen · 2021-07-27T10:44:54Z

I would like to get some more feedback on this from developers that already use modals in their apps.
Is this something we can do or it is just too much effort to remove all hacks that have been necesarry until this PR which will break implementation of Modal sizes?

jancborchardt · 2021-08-16T13:14:39Z

Less decision sounds good. :) Can you add some screenshots of different examples of modals for reference?

szaimen · 2021-08-16T18:38:53Z

Jep, Here are some examples of the different sizes :)

Size	Desktop
Small
Medium
Normal
Large
Full

Signed-off-by: szaimen <[email protected]>

skjnldsv · 2021-08-17T08:22:54Z

stable4 branch created https://github.com/nextcloud/nextcloud-vue/tree/stable4

jancborchardt · 2021-09-20T13:17:21Z

@szaimen @skjnldsv @marcoambrosini what do you think about folding the "Medium" and "Normal" into one size? I mean, the name is basically the same, so quite confusing. And then we only have small, medium, large and full – more obvious. :) The size could be something inbetween Medium and Normal.

szaimen · 2021-09-20T13:35:49Z

@jancborchardt there was already a change in the modal sizes here: #2170 (comment)
So there you can see the current catalog of sizes.

I think having both sizes could be useful in some situations. But apart from that, merging medium and normal into one size in between is fine by me.

jancborchardt · 2021-09-21T13:47:35Z

@szaimen there also doesn’t really seem to be a difference between "prompt" and "small" except the height (which could simply adjust based on content, with a max-height)?

skjnldsv · 2022-04-29T07:35:34Z

src/components/Modal/Modal.vue

+			height: calc(100% - $header-size);
+			position: absolute;
+			top: $header-size;


We could have gone for

Suggested change

height: calc(100% - $header-size);

position: absolute;

top: $header-size;

height: auto;

position: absolute;

top: $header-size;

bottom: 0;

Which imho is better than calc with $header-size :)
@szaimen

Would the result have been the same?

Yep, will have an automated height and span from top to bottom since this is absolute

Fine by me then :)

szaimen added enhancement New feature or request 3. to review Waiting for reviews design Design, UX, interface and interaction design feature: modal Related to the modal component 💥 breaking PR that requires a new major version labels Jul 20, 2021

szaimen added this to the 4.0.4 milestone Jul 20, 2021

szaimen requested review from ChristophWurst, GretaD, jancborchardt, julien-nc, marcoambrosini, miaulalala, nimishavijay and skjnldsv July 20, 2021 13:55

szaimen force-pushed the enh/noid/standardize-modal-sizes branch from fd8d14b to 633c98b Compare July 20, 2021 14:01

szaimen requested review from Pytal, artonge and juliusknorr July 20, 2021 14:34

szaimen force-pushed the enh/noid/standardize-modal-sizes branch from 633c98b to f279135 Compare July 20, 2021 16:06

skjnldsv reviewed Jul 20, 2021

View reviewed changes

src/components/Modal/Modal.vue Show resolved Hide resolved

szaimen force-pushed the enh/noid/standardize-modal-sizes branch from f279135 to 23a045e Compare July 20, 2021 16:40

szaimen force-pushed the enh/noid/standardize-modal-sizes branch 3 times, most recently from c984924 to 08da173 Compare July 20, 2021 19:36

szaimen requested a review from skjnldsv July 20, 2021 20:07

szaimen modified the milestones: 4.0.4, 5.0.0 Jul 20, 2021

szaimen force-pushed the enh/noid/standardize-modal-sizes branch from 08da173 to 2fbfd3d Compare July 24, 2021 14:38

szaimen requested a review from st3iny July 28, 2021 13:22

standardize modal sizes

b8a64f9

Signed-off-by: szaimen <[email protected]>

szaimen force-pushed the enh/noid/standardize-modal-sizes branch from 2fbfd3d to b8a64f9 Compare August 16, 2021 18:39

skjnldsv approved these changes Aug 16, 2021

View reviewed changes

Pytal approved these changes Aug 17, 2021

View reviewed changes

skjnldsv merged commit 1494de2 into master Aug 17, 2021

skjnldsv deleted the enh/noid/standardize-modal-sizes branch August 17, 2021 08:22

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 17, 2021

szaimen restored the enh/noid/standardize-modal-sizes branch August 17, 2021 15:10

szaimen deleted the enh/noid/standardize-modal-sizes branch August 17, 2021 17:51

This was referenced Aug 17, 2021

add an additional Modal size #2170

Merged

modal-container to overflow: auto #1437

Closed

set the settings modal size to large #2187

Merged

szaimen mentioned this pull request Aug 31, 2021

Improve modal and appsettingsdialog docs #2197

Merged

szaimen mentioned this pull request Sep 7, 2021

Make modal fullscreen on mobile #2215

Closed

jancborchardt mentioned this pull request Sep 21, 2021

add custom modal size option #2273

Closed

skjnldsv mentioned this pull request Apr 29, 2022

Fix design update after 5 vue components updgrade nextcloud/viewer#1229

Merged

skjnldsv reviewed Apr 29, 2022

View reviewed changes

standardize modal sizes #2126

standardize modal sizes #2126

Uh oh!

Conversation

szaimen commented Jul 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaimen Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szaimen commented Jul 20, 2021

Uh oh!

szaimen commented Jul 27, 2021

Uh oh!

jancborchardt commented Aug 16, 2021

Uh oh!

szaimen commented Aug 16, 2021

Uh oh!

skjnldsv commented Aug 17, 2021

Uh oh!

jancborchardt commented Sep 20, 2021

Uh oh!

szaimen commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jancborchardt commented Sep 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaimen Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

szaimen Jul 20, 2021 •

edited

Loading

szaimen commented Sep 20, 2021 •

edited

Loading

szaimen Apr 29, 2022 •

edited

Loading