This repository was archived by the owner on Mar 4, 2020. It is now read-only.
Merged
Conversation
kuzhelov
requested review from
dvdzkwsk,
levithomason,
miroslavstastny and
mnajdova
as code owners
kuzhelov
changed the title
Codecov Report
@@ Coverage Diff @@
## master #304 +/- ##
=======================================
Coverage 89.49% 89.49%
=======================================
Files 62 62
Lines 1171 1171
Branches 175 152 -23
=======================================
Hits 1048 1048
Misses 121 121
Partials 2 2Continue to review full report at Codecov.
|
jurokapsiar
approved these changes
Oct 3, 2018
mnajdova
approved these changes
Oct 3, 2018
mnajdova
reviewed
Oct 3, 2018
| onOpenChange={(e, newProps) => { | ||
| alert(`Popup is requested to change its open state to "${newProps.open}".`) | ||
| this.setState({ popupOpen: newProps.open }) | ||
| }} |
sophieH29
reviewed
Oct 3, 2018
| <ComponentExample | ||
| title="Default" | ||
| description="A default popup. Note that Popup is a controlled component, and its 'open' prop value could be changed either by parent component, or by user actions (keypress) - thus it is necessary to handle 'onOpenChanged' event. Try to press space key while button is focused to see the effect." | ||
| description="A default popup. Note that Popup is a controlled component, and its 'open' prop value could be changed either by parent component, or by user actions (e.g. key press) - thus it is necessary to handle 'onOpenChanged' event. Try to type some text into popup's input field and press ESC to see the effect." |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
TODO
This PR is aimed to address #295.
The main reason for improper response of Popup's open state on key press events was the fact that for trigger component event of click was handled two times when
clickwas initiated by key press:onClick()handler (we need this one for controlled Popup)PopupBehaviorhandlerBoth these handlers were toggling
openstate of the Popup - and, as a result, there were no visible changes to theopenstate of the Popup, as two toggles resulted inopen's initial value.The core of the problem was the fact that by introducing key handlers
PopupBehaviorhad overlapped the functionality that is already covered by browsers (specifically, specific keys handling logic). To prevent this, trigger key actions were removed fromPopupBehavior- as those are not necessary: thisPopupBehavioris designed to specifically target the case where button serves as a trigger (something like ratherButtonPopupBehavior) - and in that case there is no need to augment set of key handlers withspaceandenterkeys.