Close modals when clicking outside of them #3578
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kienstra
approved these changes
Oct 21, 2019
There was a problem hiding this comment.
Looks Good!
Hi @schlessera,
Wow, this has such great and thorough tests.
And closing the search modal in Twenty Twenty worked as expected:
| 'on' => 'event:action;other-event:other-action', | ||
| 'some-attribute' => 'value-a', | ||
| ], | ||
| ], |
There was a problem hiding this comment.
Nice, very thorough testing.
There was a problem hiding this comment.
Tests cover all the possible scenarios I can think of, and are thorough.
| * @param DOMElement $to DOM element to copy the attributes to. | ||
| * @param string $default_separator Default separator to use for multiple values if the attribute is not known. | ||
| */ | ||
| public static function copy_attributes( $attributes, DOMElement $from, DOMElement $to, $default_separator = ',' ) { |
There was a problem hiding this comment.
Good idea to create this function.
pierlon
approved these changes
Oct 22, 2019
| 'on' => 'event:action;other-event:other-action', | ||
| 'some-attribute' => 'value-a', | ||
| ], | ||
| ], |
There was a problem hiding this comment.
Tests cover all the possible scenarios I can think of, and are thorough.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds a set of AMP actions to the
<amp-lightbox>element that is used to replace the TwentyTwenty modals. Basically, the<amp-lightbox>is yet another "toggle" to close the modal.As there's another
.modal-innerelement that contains all the actionable elements of the modal including its container, the<amp-lightbox>is effectively the background in the AMP context.This PR offered a few technical challenges that made the solution non-trivial and required adding two helper methods:
AMP_DOM_Utils::copy_attributes()is used to copy a set of attributes from one element to the other. This helper method makes it easier in our case to move all the relevant bits from the original modal element to the<amp-lightbox>replacements. This was previously just a class attribute, but is now the following attributes instead:'class','on'(for moving the actions over),'data-toggle-target'(which is needed for the generic toggle functionality to know what to target).AMP_DOM_Utils::merge_amp_actions()is added (and used as part ofAMP_DOM_Utils::add_amp_action()) to allow for smart deduplication of AMP actions. This is now needed as the<amp-lightbox>is turned into a toggle as well, while already being a modal. As the toggle functionality addstoggleClass()actions for the'active'class on both toggles and their modals, we ended up with a duplicate toggle on the modal, which cancel each other out. The deduplication inmerge_amp_actions()makes sure this doesn't happen.Both helper methods are generic tools that prove to be useful and come with tests, so they were added to
AMP_DOM_Utils, and not as internal helpers for theAMP_Core_Theme_Sanitizer.Fixes #3570
Checklist