[fixed] Multiple modals on page causing contradictory class updates #328

fuller · 2017-02-17T08:37:28Z

Changes proposed:

Modals should only update if they are either opening or closing. If they will remained closed then there is no reason to run the renderPortal() method.

Used the tests from #326

Acceptance Checklist:

All commits have been squashed to one.
The commit message follows the guidelines in CONTRIBUTING.md.
Documentation (README.md) and examples have been updated as needed.
If this is a code change, a spec testing the functionality has been added.
If the commit message has [changed] or [removed], there is an upgrade path above.

* Add linting * Make specs pass linter * Make lib/helpers pass linter * Make lib/components pass linter This also does some signfiicant refactoring of the code to appease the linter's pro-ES2015+ stance. closes #289 closes #286 * Make travis run the lint task as well as specs closes #284

* [fixed] Make use of es6 modules * [fixed] Fix `this` scope

* Add sauce labs testing info to karma This also removes Node versions 4, 5, 6 from Travis. The node version only matters for development not for use. I don't think it's a problem officially supporting only the latest node for development. * Make specs work under IE 11

this PR allow a stack of modals to give back focus to parent modal.

coveralls · 2017-02-17T09:09:02Z

Coverage decreased (-0.9%) to 79.57% when pulling 6098432 on ajfuller:feature/isOpen-false-no-update into ebec638 on reactjs:master.

coveralls · 2017-02-17T09:20:46Z

Coverage decreased (-0.6%) to 79.928% when pulling 2e92ab8 on ajfuller:feature/isOpen-false-no-update into ebec638 on reactjs:master.

coveralls · 2017-02-17T09:31:28Z

Coverage decreased (-0.6%) to 79.928% when pulling 2e92ab8 on ajfuller:feature/isOpen-false-no-update into ebec638 on reactjs:master.

diasbruno

Unfortunately regarding 'ReactModal__Body--open', it won't fix the case where many modals are stacked and the class should be removed only when the stack is empty.

diasbruno · 2017-02-17T13:19:19Z

specs/Modal.spec.js

+    renderModal({ isOpen: true });
+    expect(classList.contains(expectedBodyClass)).toBe(true);
+    unmountModal();
+    expect(classList.contains(expectedBodyClass)).toBe(false);


Here is the problem. 2 modals were opened, one of those is unmounted, so document.body should actually have the class ReactModal__Body--open.

The class should be removed only if all modals were closed.

Ahh you're right I reversed that test inadvertently 😿

So the design decision is to allow for modal stacking then? I naively assumed from both an accessibility and UX perspective it should only allow one modal at a time to show. Would there be any consideration to disallowing stacking or is that firmly part of the component?

Thanks for the review and help understanding!

I don't see any accessibility and UX problem on stacking modals (regarding that we get to manage the focus and open/exit modals correctly).

Modal should block the UI for a single context or scope (my view). Different from a window or popup.

Hope it make sense.

I think the correct fix for this issue is to have some sorta manager to deal with this.

We have pretty strict guidelines around modals not being stackable, but I understand that it is not a universal guideline, so keeping the component flexible makes sense.

I'll likely continue to use the shouldComponentUpdate approach within my composition of the component, but feel free to close if it's not the direction you're looking for 👍

If using redux or a normal component state, you can set an state to avoid opening more modals at the same time.

Just to illustrate the idea:

{ currentModal: null } // can open

{ currentModal: 'my-modal-1' } // already in use

diasbruno · 2017-03-02T03:52:07Z

Related issue #335.

AndersDJohnson · 2017-04-11T14:20:08Z

Relates to #358.

diasbruno · 2017-06-16T00:03:46Z

@ajfuller Thank you so much for the attention to this.

claydiffrient and others added 15 commits December 31, 2016 14:49

Add code climate and code coverage

a09ff8d

Add language for CC duplication engine

721cc1e

Setup coveralls and fix some code climate stuff

3d6142c

Add badges to readme

e10181d

[fixed] Fix this scope (#296)

d50918c

* [fixed] Make use of es6 modules * [fixed] Fix `this` scope

Add sauce labs support

b261459

Get rid of CommonJS module (#299)

a776118

Modify the sample code to es2015 syntax in README.md (#295)

8108173

Updates License (#303)

ad9e115

Add sauce labs reporter for better SL statuses

0dc486a

[fixed] Infinite loop in tab scoping

75a1aa3

[fixed] Enable click to close in iOS (#301) (#304) (#313)

6ba1c8d

[change] improve reliability on focus management.

ebec638

this PR allow a stack of modals to give back focus to parent modal.

fuller mentioned this pull request Feb 17, 2017

[fixed] keep references of modals when available. #326

Closed

5 tasks

Do not update closed modal if next props are also closed

2e92ab8

diasbruno reviewed Feb 17, 2017

View reviewed changes

diasbruno added the discussion label Mar 3, 2017

diasbruno force-pushed the master branch from 6442387 to ec6b8e5 Compare June 15, 2017 21:27

diasbruno closed this Jun 16, 2017

fuller mentioned this pull request Jun 19, 2017

[fixed] Eliminate unnecessary renders when modal is closed #431

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fixed] Multiple modals on page causing contradictory class updates #328

[fixed] Multiple modals on page causing contradictory class updates #328

Uh oh!

fuller commented Feb 17, 2017 •

edited

Loading

Uh oh!

coveralls commented Feb 17, 2017

Uh oh!

coveralls commented Feb 17, 2017

Uh oh!

coveralls commented Feb 17, 2017

Uh oh!

diasbruno left a comment

Uh oh!

diasbruno Feb 17, 2017

Uh oh!

fuller Feb 17, 2017

Uh oh!

diasbruno Feb 17, 2017 •

edited

Loading

Uh oh!

diasbruno Feb 17, 2017

Uh oh!

fuller Feb 17, 2017

Uh oh!

diasbruno Feb 17, 2017

Uh oh!

diasbruno commented Mar 2, 2017

Uh oh!

AndersDJohnson commented Apr 11, 2017

Uh oh!

diasbruno commented Jun 16, 2017

Uh oh!

Uh oh!

[fixed] Multiple modals on page causing contradictory class updates #328

[fixed] Multiple modals on page causing contradictory class updates #328

Uh oh!

Conversation

fuller commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 17, 2017

Uh oh!

coveralls commented Feb 17, 2017

Uh oh!

coveralls commented Feb 17, 2017

Uh oh!

diasbruno left a comment

Choose a reason for hiding this comment

Uh oh!

diasbruno Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

fuller Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

diasbruno Feb 17, 2017 • 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!

diasbruno Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

fuller Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

diasbruno Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

diasbruno commented Mar 2, 2017

Uh oh!

AndersDJohnson commented Apr 11, 2017

Uh oh!

diasbruno commented Jun 16, 2017

Uh oh!

Uh oh!

fuller commented Feb 17, 2017 •

edited

Loading

diasbruno Feb 17, 2017 •

edited

Loading