Skip to content

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

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

Closed
wants to merge 16 commits into from

Conversation

diasbruno
Copy link
Collaborator

@diasbruno diasbruno commented Feb 15, 2017

keeping a reference of all modals in order to manage
when to add/remove classes and aria from elements
correctly.

solution based on @MarkMurphy's idea #308, but it uses a simple list
to work on IE (don't really support Set).

Fixes #218 #231 #308.

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.

claydiffrient and others added 15 commits December 31, 2016 14:49
* 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 reactjs#289
closes reactjs#286

* Make travis run the lint task as well as specs

closes reactjs#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.
@diasbruno diasbruno added the bug label Feb 15, 2017
keeping a reference of all modals in order to manage
when to add/remove classes and aria from elements
correctly.
@diasbruno diasbruno force-pushed the fix/modal-reference-count branch from 55fac94 to 09880af Compare February 15, 2017 23:28
@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 81.271% when pulling 09880af on diasbruno:fix/modal-reference-count into ebec638 on reactjs:master.

@diasbruno diasbruno removed the bug label Feb 16, 2017
@diasbruno diasbruno changed the title [fix] keep references of modals when available. [fixed] keep references of modals when available. Feb 16, 2017
@fuller
Copy link
Contributor

fuller commented Feb 17, 2017

Hi @diasbruno, I've taken your tests from this PR and proposed an alternative solution here: #328

Please let me know if you have any questions or if I'm missing a case we should be testing for 😸

@diasbruno
Copy link
Collaborator Author

@ajfuller awesome. I'll check it.

@boonier
Copy link

boonier commented Mar 2, 2017

@diasbruno thanks for the heads up on this. Any idea when this update might land?

@diasbruno diasbruno added this to the 1.7 milestone Mar 2, 2017
@diasbruno diasbruno added the bug label Mar 3, 2017
@ChenRoth-old
Copy link

can you resolve the branch conflict and submit this?

@diasbruno
Copy link
Collaborator Author

diasbruno commented Mar 14, 2017

@ChenRoth this needs to be ported to v1.
[Edit] actually, i'll close this PR and redo it to target v1.

@diasbruno diasbruno changed the base branch from master to v1 March 14, 2017 02:05
@diasbruno diasbruno closed this Mar 14, 2017
@diasbruno diasbruno changed the base branch from v1 to master March 14, 2017 02:06
@boonier
Copy link

boonier commented Mar 22, 2017

@diasbruno this hasn't been released yet has it?

@diasbruno
Copy link
Collaborator Author

@boonier I'll try to write the backport to v1 today.

@diasbruno
Copy link
Collaborator Author

@boonier PR is #358. Please, if you have time, review the PR.

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

Successfully merging this pull request may close these issues.

8 participants