Skip to content
This repository has been archived by the owner on Feb 7, 2019. It is now read-only.

Restore focus after closing modals #249

Merged
merged 1 commit into from Nov 1, 2017
Merged

Restore focus after closing modals #249

merged 1 commit into from Nov 1, 2017

Conversation

jimporter
Copy link
Contributor

@jimporter jimporter commented Oct 26, 2017 •
edited by devinreams

This partially fixes #133. Maybe it's worth adding a test, but on some level it would just be testing that react-modal works right (also, it might make more sense for such a test to be in our not-yet-existent integration tests).

@ghost ghost assigned jimporter Oct 26, 2017
@ghost ghost added the in progress We are actively working on it. label Oct 26, 2017
@jimporter jimporter removed the request for review from linuxwolf October 26, 2017 23:19
@jimporter
Copy link
Contributor Author

Clearing review until I make the tests happy.

@jimporter
Copy link
Contributor Author

Ok, tests should pass. There aren't any tests for the focus restoration because we can't use react-modal in our tests yet. We have to stub it because enzyme doesn't yet support React 16 portals; hence, there's currently no way to test that we're using react-modal's focus logic correctly.

@sandysage sandysage added this to the 0.1.2 milestone Oct 30, 2017
@jimporter jimporter mentioned this pull request Oct 30, 2017
Open
2 tasks
@devinreams
Copy link
Contributor

Need latest to fix tests.

Then ready for review.

@jimporter
Copy link
Contributor Author

jimporter commented Oct 31, 2017 •
edited

@devinreams @linuxwolf So, it turns out I pushed the version of my code with passing tests, but Travis decided to run the old code and mark the new code as having failed, despite them not having the same hashes. I retriggered the Travis build, so hopefully it'll sort itself out this time.

I hate Travis.

@jimporter
Copy link
Contributor Author

Ohhhh, it's because we added a new place using Modals and those tests failed. I didn't realize that Github rebased PRs onto master before submitting them to Travis.

@jimporter jimporter force-pushed the improve-focus branch 2 times, most recently from 5cd5117 to 01085f2 Compare October 31, 2017 19:57
linuxwolf
linuxwolf reviewed Oct 31, 2017
View reviewed changes
Copy link
Contributor

@linuxwolf linuxwolf left a comment

Choose a reason for hiding this comment

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

r+

@linuxwolf linuxwolf self-requested a review October 31, 2017 20:17
linuxwolf
linuxwolf previously approved these changes Oct 31, 2017
View reviewed changes
Copy link
Contributor

@linuxwolf linuxwolf left a comment

Choose a reason for hiding this comment

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

r+ again ... with the correct radio selected.

@sandysage sandysage added the chore label Nov 1, 2017
@jimporter jimporter merged commit 6aa034a into mozilla-lockwise:master Nov 1, 2017
@ghost ghost removed the in progress We are actively working on it. label Nov 1, 2017
@jimporter jimporter deleted the improve-focus branch November 1, 2017 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@linuxwolf linuxwolf linuxwolf left review comments

Assignees

@jimporter jimporter

Labels
chore pi-verified
Projects
None yet
Milestone
0.1.2
Development

Successfully merging this pull request may close these issues.

Focus is lost when focused element goes away
5 participants
@jimporter @devinreams @linuxwolf @m8ttyB @sandysage