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] Allow different modal positions on multi-modals. Added top and bottom aligned modal #532

Merged
merged 5 commits into from
Mar 3, 2019

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Mar 1, 2019

Description

When a modal opened another modal, the positioning given by centered:false was broken. The first modal did not stay at it's supposed position.
Reason for this was, that modal positioning was done by applying top aligned to the page dimmer instead of the modal itself.

This PR now takes care of the positioning for each opened modal and keeps its position until it's closed.
In addition it also invents top aligned modal and bottom aligned modal

Hint: Unfortunately the current code already used the word top internally to mark the most active modal in the front and removed it again when the modal closes. No CSS Class for top was existing and it is not even documented. I therefore decided to rename the internal topto frontinstead, because i didn't want to name this upper aligned just to keep the internal top setting. top aligned is used everywhere else in the framework.

But, as i suggest this PR to be part of the next 2.7.x Release we can do one of 2 things:

  1. Accept the PR as it is because the top-logic was only used internally, temporary and hidden undocumented
  2. Consider this a breaking change. To avoid waiting for a major release, i would revert the settings for "front" and just change the current .top.aligned CSS settings to simply .aligned to still fix the belonging issues. I would then prepare another PR after the changes are merged to change back to "front" and ".top.aligned" for the next major release

Testcase

Click the button inside the modal to open the second one and watch how the modals ...

Broken

... change their position
https://jsfiddle.net/9pnmzgkc/

Fixed

... stay at their supposed position
https://jsfiddle.net/phk50two/

Additional Examples for top aligned modal and bottom aligned modal

https://jsfiddle.net/5ahkg7t1/

Alltogether

https://jsfiddle.net/4dxfsctj/

Multimodal with different Position

https://jsfiddle.net/46wLg1nu/

The modals with individual positions called from each other

https://jsfiddle.net/fpk01L54/

Screenshot

3inheritmodals

Closes

#530
Semantic-Org/Semantic-UI#6677
Semantic-Org/Semantic-UI#5883

@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/css Anything involving CSS lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews state/awaiting-docs Pull requests which need doc changes/additions tag/sui-issue Taken from an existing Issue/PR of SUI labels Mar 1, 2019
@lubber-de lubber-de added this to the 2.7.x milestone Mar 1, 2019
y0hami
y0hami previously approved these changes Mar 2, 2019
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

I think this is fine and we can get away with changing top to front, I don't think its something people change especially since its undocumented.

@dutrieux
Copy link
Contributor

dutrieux commented Mar 2, 2019

Before my questions : thx for all your works !!! 👍 👍 👍

With this sample "Alltogether" https://jsfiddle.net/b1sa5863/2/

When you resize the windows, the center modal is display on the top : (I check on firefox and Chrome)
dialog

And I have a other question why there aren't dimmer on this sample, 3 modals are open but without dimmer on the first two ?

@lubber-de
Copy link
Member Author

lubber-de commented Mar 2, 2019

@dutrieux 🤔 Nice catch...i'll investigate.
And the reason why those modals dont't have a separate dimmer is, because the example opens the modals independently of each other. The dimmer occurs when a modal is called within the other modal.
When i have fixed the resize issue i'll prepare another example. I am currently on the road so i might only have time in the evening to look into all of this

@lubber-de
Copy link
Member Author

@dutrieux Fixed it 😁 See altogether example here https://jsfiddle.net/fLbzpeun/

@lubber-de lubber-de added the state/on-hold Issues and pull requests which are on hold for any reason label Mar 2, 2019
@lubber-de
Copy link
Member Author

I still have to investigate into the whole center variant, so i've put this on hold...as said, i'll take care of it this evening 😉

@dutrieux
Copy link
Contributor

dutrieux commented Mar 2, 2019

I check https://jsfiddle.net/fLbzpeun/ and there is a problem on the bottom modal when you resize the window

dialog

And the modal center is blurred : the position is change by 1 pixel on y-axis too fast (or something like that) :
Modal 2 on center position :
blurred
Modal 2 on top position :
not_blurred

@lubber-de
Copy link
Member Author

Yes, the new bottom modal was the reason. I think I fixed it with the latest commit. I still have to prepare the example fiddles again

@lubber-de lubber-de removed the state/on-hold Issues and pull requests which are on hold for any reason label Mar 2, 2019
@lubber-de
Copy link
Member Author

lubber-de commented Mar 2, 2019

@dutrieux It should be finally fixed now 🙏
I adjusted all examples in the original description, so for the resize issue try "altogether" here https://jsfiddle.net/vsgnymp0/ now
I also added a new example showing the wanted extra dimmer on the two other modals.Also changed the screenshot in the decription accordingly. As you will see, this time the modals are called from each other to trigger the blocking dimmers.
https://jsfiddle.net/jnw6ho78/

@hammy2899 Please review again

@lubber-de lubber-de requested a review from y0hami March 2, 2019 19:48
y0hami
y0hami previously approved these changes Mar 2, 2019
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

@dutrieux
Copy link
Contributor

dutrieux commented Mar 2, 2019

I fund something strange with your sample : https://jsfiddle.net/jnw6ho78/

When I open and close all modals the first time, there is no problem, but when I open and close all modals an other time, the scrollbar appears briefly :

dialog

I have the same problem on Chrome & Firefox & Edge

@lubber-de
Copy link
Member Author

@dutrieux ok, hopefully the last needed fix 😅
Please try https://jsfiddle.net/fpk01L54/
The scrollbar should not appear anymore

@lubber-de lubber-de requested a review from y0hami March 2, 2019 23:47
@dutrieux
Copy link
Contributor

dutrieux commented Mar 2, 2019

Very nice, that work perfectly : juste one question, in your sample https://jsfiddle.net/fpk01L54/ and on modification of Modal.js, there is not this fix #427 too ?

@lubber-de
Copy link
Member Author

lubber-de commented Mar 3, 2019

The fix for #427 Is only partially implemented in the example for this PR because the fix for #427 needs more css and adjusted dimmer.js to be included in a fiddle. #427 is part of upcoming 2.7.3, so if this PR gets approved and merged also then both fixes will be released by 2.7.3

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

@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 Mar 3, 2019
@lubber-de lubber-de modified the milestones: 2.7.x, 2.7.3 Mar 3, 2019
@y0hami y0hami merged commit 0387e30 into fomantic:develop Mar 3, 2019
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Mar 3, 2019
y0hami pushed a commit to fomantic/Fomantic-UI-Docs that referenced this pull request Mar 12, 2019
Docs for new `top aligned` and `bottom aligned` modal variants as of fomantic/Fomantic-UI#532
@y0hami y0hami mentioned this pull request Apr 1, 2019
@lubber-de lubber-de deleted the fix/530/multimodal_positioning branch April 2, 2019 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS lang/javascript Anything involving JavaScript state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo tag/sui-issue Taken from an existing Issue/PR of SUI type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants