This repository has been archived by the owner on Mar 4, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 55
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
kuzhelov
requested review from
dvdzkwsk,
levithomason,
miroslavstastny and
mnajdova
as code owners
October 3, 2018 09:33
kuzhelov
changed the title
fix(Popup) keypress toggle logic
fix(Popup): keypress toggle logic
Oct 3, 2018
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 2 Continue 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 }) | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for simplicity :)
sophieH29
reviewed
Oct 3, 2018
@@ -7,7 +7,7 @@ const Types = () => ( | |||
<ExampleSection title="Types"> | |||
<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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like it!
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
click
was initiated by key press:onClick()
handler (we need this one for controlled Popup)PopupBehavior
handlerBoth these handlers were toggling
open
state of the Popup - and, as a result, there were no visible changes to theopen
state of the Popup, as two toggles resulted inopen
's initial value.The core of the problem was the fact that by introducing key handlers
PopupBehavior
had 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: thisPopupBehavior
is 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 withspace
andenter
keys.