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

Reveal anchor focus fix #10711

Merged
merged 5 commits into from
Dec 25, 2017
Merged

Conversation

Owlbertz
Copy link
Contributor

@Owlbertz Owlbertz commented Oct 7, 2017

Focus active anchor rather than last one in collection when closing a Reveal modal.

Addresses #10604.

@IamManchanda IamManchanda self-requested a review October 12, 2017 15:15
Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

LGTM

@ncoden
Copy link
Contributor

ncoden commented Dec 24, 2017

IamManchanda self-requested a review on 12 Oct

@IamManchanda Would you like to review this PR or do I merge it ?

@IamManchanda
Copy link
Contributor

Taking this up @ncoden

@@ -228,6 +228,9 @@ class Reveal extends Plugin {
}
}

// Remember anchor that opened it to set focus back later, have general anchors as fallback
this.$activeAnchor = $(document.activeElement).is(this.$anchor) ? $(document.activeElement) : this.$anchor;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@IamManchanda IamManchanda left a comment

Choose a reason for hiding this comment

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

@ncoden @Owlbertz I haven't entered into testing as of yet

But visual testing is working cool and logic seems cool too!

So approved from my end... Thanks!

@IamManchanda
Copy link
Contributor

I think we should merge it @ncoden

@ncoden
Copy link
Contributor

ncoden commented Dec 25, 2017

@IamManchanda This PR do not install or update any package. Why updating the yarn lock file there ?

This reverts commit 6cfebc3.
@ncoden ncoden merged commit e4e96b0 into foundation:develop Dec 25, 2017
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
…ix for v6.5.0

9fce99e Focus active anchor rather than last one in collection when closing a Reveal modal.
9e8535a Added tests for foundation#10604.
f954f7e Added visual test for foundation#10604.
6cfebc3 Update Yarn file!
6bc8691 Revert "Update Yarn file!"

Co-Authored-By: Harry Manchanda <harmanmanchanda182@gmail.com>
Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr>
Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
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.

3 participants