Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Modal] Introducing "overlay fullscreen modal" #526

Merged
merged 3 commits into from
Feb 27, 2019

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Feb 26, 2019

Description

The current fullscreen modal was (according to the docs) supposed to "use the entire size of the screen". This was not really true, because there still was lots of margin left, so you could see the dimmer, and also modals with little content were not even close to fullscreen.

This PR makes it possible now and adds a new variant to the modal module:

overlay fullscreen modal

When used, it is now really using and overlaying the entire viewport, regardless of the content. 🙂

Testcase

https://jsfiddle.net/sbru45e1/

Screenshot

overlayfullscreenmodal

Closes

#522

@lubber-de lubber-de added type/feat Any feature requests or improvements lang/css Anything involving CSS state/awaiting-reviews Pull requests which are waiting for reviews labels Feb 26, 2019
@lubber-de lubber-de added this to the 2.7.x milestone Feb 26, 2019
@lubber-de lubber-de added the state/awaiting-docs Pull requests which need doc changes/additions label Feb 26, 2019
@y0hami
Copy link
Member

y0hami commented Feb 26, 2019

Maybe this should be entire screen modal instead of entire fullscreen modal

@lubber-de
Copy link
Member Author

@hammy2899 Mmh, don't think so. But that would also mean to add more css to the code because it currently relies on many fullscreen settings already there which we would need to double or at least add entire.screen to all existing fullscreen css settings. Unnecessary with no value IMHO

@lubber-de
Copy link
Member Author

Ok, after research there are only 4 css settings... So if everybody wants to change the name, I am fine 😊

@y0hami
Copy link
Member

y0hami commented Feb 26, 2019

@fomantic/core

@ColinFrick
Copy link
Member

For me entire screen and fullscreen seem interchangeable and don't convey the difference.

Additionally Wikipedia defines a modal as following:

a modal window is a graphical control element subordinate to an application's main window. It creates a mode that disables the main window but keeps it visible, with the modal window as a child window in front of it.

Maybe we could use a word like cover / overlay or borderless / gutterless

@y0hami
Copy link
Member

y0hami commented Feb 27, 2019

@ColinFrick that makes sense, I think cover, overlay or gutterless work the best. Maybe it should be fullscreen overlay modal since that makes the most semantic sense.

@prudho
Copy link
Contributor

prudho commented Feb 27, 2019

The fullscreen term bother me a little here. In the future we may want to display a modal into a viewport and not in the entire page.

@lubber-de
Copy link
Member Author

lubber-de commented Feb 27, 2019

Doesn't this feature cover the whole background? So fullscreen covered modal may fits better?
overlay sounds like I still can see something underneath.. But probably that's my only first impression
gutterless would be something for grid (there is a demand left to completely omit padding there)
So, if we choose gutterless we could use this naming aswell for grid (separate pr)

@y0hami
Copy link
Member

y0hami commented Feb 27, 2019

I think cover and overlay are very similar in semantic terms. Cover meaning you put something over another thing but it doesn't necessarily have to hide what your covering (you can cover the right side of a square but the left side would still be showing). Overlay somewhat means the same thing, you can overlay something but you don't necessarily need to overlay the whole thing.

I think adding fullscreen to describes it in enough detail to make sense, cover fullscreen would then mean you are covering the fullscreen. This would also work for overlay fullscreen since it would mean your putting the modal over the fullscreen.

As for gutterless I think it is more of a grid term since the modal doesn't have a gutter, the "gutter" for a modal is defined via its size unlike the grid which will always have a gutter.

@lubber-de
Copy link
Member Author

lubber-de commented Feb 27, 2019

Ok, then let's use overlay fullscreen modal. We already have overlay sidebar and overlay nag while there isn't any covered anywhere in the framework

@ColinFrick @prudho Agree?

Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lubber-de agreed !

LGTM

@lubber-de lubber-de changed the title [Modal] Introducing "entire fullscreen modal" [Modal] Introducing "overlay fullscreen modal" Feb 27, 2019
@lubber-de
Copy link
Member Author

Changed the code and PR description/screenshots/fiddle accordingly
Adjusted fiddle: https://jsfiddle.net/sbru45e1/

Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@y0hami y0hami merged commit 3f57373 into fomantic:develop Feb 27, 2019
@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Feb 27, 2019
@lubber-de lubber-de removed this from the 2.7.x milestone Feb 27, 2019
@lubber-de lubber-de added this to the 2.7.3 milestone Feb 27, 2019
@lubber-de lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Feb 27, 2019
@lubber-de lubber-de deleted the feat/522/entire_fullscreen_modal branch February 27, 2019 17:01
@y0hami y0hami mentioned this pull request Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants