feat(Popup): close popup on click by kuzhelov · Pull Request #410 · microsoft/fluent-ui-react

kuzhelov · 2018-10-31T00:54:28Z

TODO

update CHANGELOG

Close Popup on mouse click

This set of changes introduces functionality that will close Popup on click. Currently Portal is not reused, as there is some refactoring planned for this component before reusing it in Popup. At the same time, provided changes introduce full set of functionality client would expect, so this remains to be just a question of further refactoring (Portal) and generalization (reuse Portal).

EventStack subscription API adjustments

Event stack subscriptions were reworked, to make the API for subscribing and unsubscribing to be safer. Please, consider to take a look on the following comment for more details on that: #391 (comment)

codecov · 2018-10-31T00:55:57Z

Codecov Report

Merging #410 into master will decrease coverage by 1.38%.
The diff coverage is 20.58%.

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage   91.79%   90.41%   -1.39%     
==========================================
  Files          41       41              
  Lines        1329     1356      +27     
  Branches      193      198       +5     
==========================================
+ Hits         1220     1226       +6     
- Misses        105      126      +21     
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Portal/Portal.tsx	`98.18% <100%> (+1.88%)`	⬆️
src/components/Popup/Popup.tsx	`33.33% <12.9%> (-7.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34bf5f1...641361c. Read the comment docs.

src/components/Popup/Popup.tsx

sophieH29 · 2018-10-31T09:13:38Z

src/lib/eventStack/eventStack.tsx

  // ------------------------------------

-  _find = (target, autoCreate = true) => {
+  private _find = (target, autoCreate = true) => {


can we add also typings for methods' parameters, in regards to refactoring of event stack?

btw, you might consider it as separate PR

would rather focus on functionality in this PR - those haven't existed before, would like to devote PR to introduce them

@kuzhelov - I think whenever we claim we'll add something in a follow-up PR we should create tickets or issues to actually track the work so that it won't slip through the cracks and other devs have access to it and can pick it up

src/lib/eventStack/eventStack.tsx

sophieH29 · 2018-10-31T14:03:23Z

src/components/Popup/Popup.tsx

+
+  private closeAndFocusTriggerOnClickIfOpen() {
+    if (this.state.open) {
+      setTimeout(() => this.clickSubscription.replaceWith('click', this.closeAndFocusTrigger))


in your previous refactoring setTimeout was part of EventStack and, as I understood, a motivation of it was performance reason (please, correct me, if I am wrong). So the question is, should we leave it as part of the library so everyone who uses it can benefit from it? It can be as a separate method subscribeAsync.

this is a very good point. Actually, for now have decided to move this aspect to the client's code, as have discovered few additional subtle problems that may arise with eventStack's logic (and the fact that subscription handler could be called at the same JS frame).

The simplest example - suppose that there is a following client code:

foo() { // note, with 'setTimeout' this subscription will be made async const subscription = EventStack.subscribe('click', ...) // so there is, in fact, no active subscription at this point subscription.unsubscribe() }

Note that if subscription logic is async, this will result in subscription still being active at the end of the method - as 'unsubscribe' will be invoked at the moment when there is no subscription made yet.

One simplest solution would be to make all the necessary methods async, but this will complicate client's code. What I am thinking about, however, is a bit more elaborative approach where all these bits will be handled properly by eventStack, at the same time without switch to async model requested from the client (as async methods are quite error and race condition prone to be used in React event handlers). Now am working on the PR where would like to address this problem.

src/components/Popup/Popup.tsx

joheredi · 2018-10-31T08:54:30Z

src/components/Popup/Popup.tsx


  private static isBrowserContext = isBrowser()

+  private clickSubscription = EventStackSubscription.empty()


It may be a good idea to add a prop to choose if the popup should be closed onClick. We may want to have it true by default

just as a counterargument, we might argue that as long as Popup supports controlled mode, there are all the necessary tools to customize Popup's behavior.

Please, don't get me wrong - personally, see this suggestion as being absolutely reasonable. At the same time, would like to keep this PR simple and targeted to specific thing that it is aimed to address, so that we would be able to decide on this proposed prop later and, if necessary, introduce separate PR for that.

Makes sense!

sophieH29 · 2018-11-01T09:02:01Z

src/lib/accessibility/FocusZone/FocusTrapZone.tsx

    const { forceFocusInsideTrap, isClickableOutsideFocusTrap } = this.props
    if (forceFocusInsideTrap) {
-      eventStack.sub('focus', this._handleOutsideFocus, {
+      this._focusSubscription.unsubscribe()


What is the main point to call unsubscribe before actual subscription? Can't we assure that we are unsubscribed from any events at the initialization EventStack.noSubscription?

+100 it doesn't look like a good pattern, we don't want to attempt teaching developers to do this, they will forget; I find the eventStack util difficult to use now

please, be specific about 'do what', thanks! Because see the following as a counterargument to the statement 'hard to use now':

now

// safe subscription const subscription = EventStack.subscribe('onclick', () => { .. /* processing logic here */ }) // - this object has IntelliSense support // - safe unsubscribe instruction subscription.unsubscribe()

before

// ooops eventStack.sub('onclick', () => { ... }) // ooops eventStack.sub('onclick', this.handleClick.bind(this)) eventStack.sub('onclick', this.handleClickWithBoundThis, { useCapture: true }) // oooops // - what were the arguments I've subscribed with? // - some of them are critical for proper unsubscription eventStack.unsub('onclick', this.handleClickWithBoundThis)

Sorry, but now cannot agree with the point mentioned

I agree that it looks much better now, that it was. The only my concern was about calling unsubscribe before actual subscribe, as it seems to me not very intuitive.

The only my concern was about calling unsubscribe before actual subscribe, as it seems to me not very intuitive.

Here I would note that this is not something that is directly eventStack-related, as what we are doing is the following:

we would like to subscribe and store subscription in the object's field

there is (maybe) a pending subscription stored in this field - thus, we need to unsubscribe for it

as it is safe to call 'unsubscribe' on 'noSubscription', we are not making any null checks for it. If null value will be used as initialiser, we would write something like subscription && subscription.unsubscribe()

then we are making new subscription, storing reference on it

*Note that initially the following API was suggested for it, but we've agreed to cancel/postpone it:

// terminates previous subscription and replaces with new one this.clickSubscription.replaceWith('click', ...)

bmdalex

pls take a look at comments

bmdalex · 2018-11-01T13:17:03Z

src/lib/accessibility/FocusZone/FocusTrapZone.tsx

    const { forceFocusInsideTrap, isClickableOutsideFocusTrap } = this.props
    if (forceFocusInsideTrap) {
-      eventStack.sub('focus', this._handleOutsideFocus, {
+      this._focusSubscription.unsubscribe()


+100 it doesn't look like a good pattern, we don't want to attempt teaching developers to do this, they will forget; I find the eventStack util difficult to use now

bmdalex · 2018-11-01T14:12:57Z

src/lib/eventStack/eventStack.tsx

  // ------------------------------------

-  _find = (target, autoCreate = true) => {
+  private _find = (target, autoCreate = true) => {


@kuzhelov - I think whenever we claim we'll add something in a follow-up PR we should create tickets or issues to actually track the work so that it won't slip through the cracks and other devs have access to it and can pick it up

src/lib/eventStack/eventStack.tsx

src/components/Popup/Popup.tsx

mnajdova

I briefly took a loot into this, but seems that other developers already review it. Approving as all comments are resolved.

kuzhelov-ms added 3 commits October 31, 2018 01:28

close popup on outside click

32981cd

remove eventStack async logic

bc3909b

simplify Popup examples

04e11a7

kuzhelov requested review from dvdzkwsk, levithomason, miroslavstastny and mnajdova as code owners October 31, 2018 00:54

kuzhelov added the 🚀 ready for review label Oct 31, 2018

kuzhelov changed the title ~~feat(Popup): close popup on outside area click~~ feat(Popup): close popup on click Oct 31, 2018

sophieH29 reviewed Oct 31, 2018

View reviewed changes

src/components/Popup/Popup.tsx Outdated Show resolved Hide resolved

sophieH29 reviewed Oct 31, 2018

View reviewed changes

src/lib/eventStack/eventStack.tsx Outdated Show resolved Hide resolved

alinais mentioned this pull request Oct 31, 2018

Popup: close popup when user clicks outside of the popup #307

Closed

sophieH29 reviewed Oct 31, 2018

View reviewed changes

joheredi reviewed Oct 31, 2018

View reviewed changes

kuzhelov-ms added 5 commits October 31, 2018 18:09

introduce recommended API changes

e15f2de

do not close popup when clicked inside popup area

bd8e57b

simplify handler logic

a167b1f

remove unnecessary unsub instruction

0ccef7b

remove state that was previously used in controlled mode

3d12b10

jmanuelh approved these changes Oct 31, 2018

View reviewed changes

joheredi approved these changes Oct 31, 2018

View reviewed changes

sophieH29 reviewed Nov 1, 2018

View reviewed changes

bmdalex suggested changes Nov 1, 2018

View reviewed changes

miroslavstastny added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Nov 5, 2018

bmdalex approved these changes Nov 7, 2018

View reviewed changes

kuzhelov added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Nov 8, 2018

kuzhelov and others added 5 commits November 8, 2018 03:59

Merge branch 'master' into feat/popup-close-on-outside-click

e34c67d

update changelog

fd3ccb4

make method name to be more descriptive

cac5a48

properly maintain Popup state on component updates

af5998f

rename method to better reflect semantics

851c81f

kuzhelov added ready for merge and removed 🚀 ready for review labels Nov 8, 2018

mnajdova approved these changes Nov 8, 2018

View reviewed changes

Merge branch 'master' into feat/popup-close-on-outside-click

641361c

kuzhelov merged commit affbec0 into master Nov 8, 2018

layershifter deleted the feat/popup-close-on-outside-click branch November 9, 2018 09:23


		private static isBrowserContext = isBrowser()

		private clickSubscription = EventStackSubscription.empty()

Conversation

kuzhelov commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Close Popup on mouse click

EventStack subscription API adjustments

Uh oh!

codecov bot commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmdalex Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

now

before

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmdalex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmdalex Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kuzhelov commented Oct 31, 2018 •

edited

Loading

codecov bot commented Oct 31, 2018 •

edited

Loading

bmdalex Nov 1, 2018 •

edited

Loading

kuzhelov Oct 31, 2018 •

edited

Loading

kuzhelov Oct 31, 2018 •

edited

Loading

kuzhelov Nov 7, 2018 •

edited

Loading

kuzhelov Nov 7, 2018 •

edited

Loading

bmdalex Nov 1, 2018 •

edited

Loading