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

fix(Modal): remove dimmer={false} #2882

Merged
merged 5 commits into from
Jun 9, 2018

Conversation

mihai-dinculescu
Copy link
Member

@mihai-dinculescu mihai-dinculescu commented Jun 5, 2018

Fixes #2873.

In fact, this change should be done in v0.81.0 when we performed update to SUI 2.3 (#2657). Without a dimmer, it is no longer able to figure out the position of the modal.

A Special Message about Flex Modals

There will be an update shortly to resolve issues related to flex modals when using multiple modals and detachable: false, in order to not hold up this release, we've decided to move forward without a fix.

A general solution will most likely require branching code for IE11 which will disable flex (as IE11 doesnt correctly implement the latest spec for absolute positioned flex containers).

Before

You able to hide a dimmer with: <Modal dimmer={false} />.

After

This behaviour is deprecated and removed, you need to apply custom styling for dimmers if you actually need this feature.


Changes

1. Multiple example

Remove the now controversial dimmer={false}. It adds nothing to the example anyway.

2. Close Config example

Make it so that the "Yes" and "No" buttons inside the Modal can close it. The "No Close on Dimmer Click" example is currently unclosable and requires browser refresh.

3. Dimmer examples

Removed useless example with dimmer={false}.

4. Remove useless tests and update propTypes for the dimmer prop

Mihai Dinculescu and others added 2 commits June 5, 2018 16:31
@layershifter layershifter changed the title docs(Modal): Fix multiple and close examples BREAKING(Modal): remove dimmer={false} Jun 6, 2018
@codecov-io
Copy link

codecov-io commented Jun 6, 2018

Codecov Report

Merging #2882 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2882   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files         161      161           
  Lines        2733     2733           
=======================================
  Hits         2724     2724           
  Misses          9        9
Impacted Files Coverage Δ
src/modules/Modal/Modal.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a26279...bf62086. Read the comment docs.

@layershifter
Copy link
Member

@mihai-dinculescu thanks for PR 👍 I've pushed some updates and update the PR's description

Copy link
Member Author

@mihai-dinculescu mihai-dinculescu left a comment

Choose a reason for hiding this comment

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

Whops. It was already done. Sorry about that.

@levithomason
Copy link
Member

I believe this is actually a bug fix to 0.80.1, correct? It should have been shipped as a breaking change in the 0.81.0 release, but now that we've missed that I am inclined to say we're "fixing" the 0.81 release.

Is this sane to others?

@layershifter
Copy link
Member

layershifter commented Jun 6, 2018

In fact, dimmer={false} already is not working, so we can ship this as a fix. I updated title 👍

@layershifter layershifter changed the title BREAKING(Modal): remove dimmer={false} fix(Modal): remove dimmer={false} Jun 6, 2018
@layershifter
Copy link
Member

layershifter commented Jun 9, 2018

Performed a review again, merged. @mihai-dinculescu thanks 👍

@levithomason
Copy link
Member

Released in semantic-ui-react@0.81.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modal: dimmer={false} stops the modal from opening
4 participants