feat(Popup): refactor and add controlled mode #282

kuzhelov · 2018-09-26T12:50:23Z

TODO

add CHANGELOG entry
add CHANGELOG entry for the feat(Accessibility): Add accessibility behavior to Popup #218

The following issues are aimed to be addressed by the proposed PR:

simplify logic of Popup component
- reduce amount of abstraction layers (primary reason for that is to simplify logic of refs capturing)
- eliminate unnecessary props
address subtle problems with logic
- it turns out that in RTL mode we were 'flipping' CSS position properties three times (instead of just one), and this RTL positioning could be easily broken with custom component/DOM element being provided as value for as prop
- custom as components/elements could cause breaks in ref capturing logic
introduce controlled mode to Popup
- by adding open and defaultOpen props

codecov · 2018-09-26T12:51:44Z

Codecov Report

Merging #282 into master will decrease coverage by 1.79%.
The diff coverage is 24%.

@@           Coverage Diff            @@
##           master    #282     +/-   ##
========================================
- Coverage   91.69%   89.9%   -1.8%     
========================================
  Files          62      62             
  Lines        1168    1159      -9     
  Branches      150     150             
========================================
- Hits         1071    1042     -29     
- Misses         94     115     +21     
+ Partials        3       2      -1

Impacted Files	Coverage Δ
src/components/Popup/PopupContent.tsx	`100% <ø> (ø)`	⬆️
src/components/Popup/Popup.tsx	`42% <24%> (-41.93%)`	⬇️

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 3e5c4fb...b7dd143. Read the comment docs.

sophieH29 · 2018-09-27T08:23:56Z

docs/src/examples/components/Popup/Variations/PopupExamplePosition.shorthand.tsx

@@ -1,145 +1,67 @@
 import React from 'react'
 import { Button, Grid, Popup } from '@stardust-ui/react'

+class PopupArrowExample extends React.Component<any, any> {


why the name is PopupArrow? :)

+1 name should reflect name of file

sophieH29 · 2018-09-27T08:27:59Z

src/components/Popup/Popup.tsx

    /** The popup content (deprecated). */
-    children: customPropTypes.disallow(['children']),
+    children: PropTypes.any, // customPropTypes.disallow(['children']),


is it intended?

to remove disallow statement - yes, this is intended :) but at the same time (other than cleaning up commented code), PropTypes.node would be a better substitute here - let me address that.

why do we remove it? we support children API now?

yes - as this is quite reasonable for the Popup case, see:

<SomeTree> <Popup content={popupContent}> <button>Click me to trigger popup</button> </Popup> </SomeTree>

Now to introduce popup functionality it is necessary just to wrap the component that previously was in the tree - to augment it with popup functionality (and make it a trigger)

sophieH29 · 2018-09-27T09:15:39Z

src/components/Popup/Popup.tsx

+    toggle: e => _.invoke(this.props, 'onOpenChange', e, !this.props.open),
+    closeAndFocusTrigger: e => {
+      _.invoke(this.props, 'onOpenChange', false)
+      _.invoke(this.state.triggerRef, 'focus')


So, in case of user won't provide onOpenChange function which will handle open/close state, the Popup won't be closed, but the trigger would be focused. It seems like a not expected scenario.
And seems like same for toggle (in case it's not a button).
Let's discuss it.

this is a good catch - lets discuss closeAndFocus case. Speaking of the toggle one - could you, please, suggest what the problem is exactly about?

Speaking of a toggle, if a trigger is not button element, you won't be able to toggle popup with Enter\ Space button. So, do we expect here that a user should use ```onOpenChange`` function in case to handle toggling?

yes - but this leads us to more general problem that is rather accessibility related. In this PR I would like to focus on the following things, with all the previously introduced functionality being untouched:

refactor and simplify Popup base implementation

introduce controlled mode to it

So, these are the main reasons I've left PopupBehavior's functionality as it was introduced before. so that If we would like to address/discuss some problems related to this aspect exclusively, we would be able to do it by means of a dedicated PR.

Please, let me know about your thoughts on that :)

src/components/Popup/Popup.tsx

docs/src/examples/components/Popup/Variations/PopupExamplePosition.shorthand.tsx

bmdalex · 2018-09-27T12:12:36Z

src/components/Popup/Popup.tsx

  children?: ReactChildren
  className?: string
  content?: ItemShorthand | ItemShorthand[]
+  defaultOpen?: boolean
+  open?: boolean
+  onOpenChange?: () => void


should't we expose the open flag here as well; what's the advantage of this on having 2 props as in SUIR:
onOpen and onClose?

+1 for adding handlers onOpen and onClose that would be invoked based on the value of the open. It is much cleaner API for the user.

any thoughts here?

bmdalex · 2018-09-27T12:15:18Z

src/components/Popup/Popup.tsx

+            ...accessibility.keyHandlers.trigger,
+          })}
+        </Ref>
+        {this.props.open && this.renderPopupContent(rtl, accessibility)}


nit: add open to destructured object above and use from there

src/components/Popup/Popup.tsx

docs/src/examples/components/Popup/Variations/PopupExamplePosition.shorthand.tsx

bmdalex

pls take a look at comments

bmdalex · 2018-09-27T12:37:38Z

src/components/Popup/Popup.tsx

+          style={popupPlacementStyles}
+          {...accessibility.attributes.popup}
+          {...accessibility.keyHandlers.popup}
+          data-placement={placement}


why is this needed? accessibility / w3c standards? it's not adding any functionality

these lines reflect changes that were merged by another PR - please, take a look on the current state of master. So, those are necessary to project this already introduced functionality on top of changes made in this PR

I was talking about line 162:

data-placement={placement}

oh, got it - let me address..

mnajdova · 2018-10-01T09:53:09Z

docs/src/examples/components/Popup/Variations/PopupExamplePosition.shorthand.tsx

+  { position: 'after', align: 'top', icon: 'arrow circle right', padding: '5px 5px 18px 42px' },
+  { position: 'after', align: 'center', icon: 'arrow circle right', padding: '5px 5px 5px 42px' },
+  { position: 'after', align: 'bottom', icon: 'arrow circle right', padding: '18px 5px 5px 42px' },
+]

 const PopupExamplePosition = () => (


This const and the class both have the same name which is casing some compiler errors:

[at-loader] Checking finished with 2 errors [at-loader] ./docs/src/examples/components/Popup/Variations/PopupExamplePosition.shorthand.tsx:4:7 TS2300: Duplicate identifier 'PopupExamplePosition'. [at-loader] ./docs/src/examples/components/Popup/Variations/PopupExamplePosition.shorthand.tsx:52:7 TS2300: Duplicate identifier 'PopupExamplePosition'.

Please rename one of them.

mnajdova · 2018-10-01T10:04:40Z

I think there is some issue with the RTL. Please take a look into the gif:

kuzhelov · 2018-10-01T10:35:07Z

I think there is some issue with the RTL. Please take a look into the gif:

This is behavior that is not introduced by this PR, but rather the one that we currently have. Thanks @mnajdova for spotting it - let me, please, open an issue: #288

bmdalex

approved

kuzhelov-ms added 6 commits September 26, 2018 00:28

first round of changes

969e848

introduce auto controlled 'open' prop to Popup

8dab68c

adjust examples code

95125a3

introduce defaultOpen prop

353f870

fix unit tests

c46d443

address lint issues

583b6de

kuzhelov requested review from dvdzkwsk, levithomason, miroslavstastny and mnajdova as code owners September 26, 2018 12:50

kuzhelov changed the title ~~Feat/refactor popup~~ feat(Popup): refactor and add controlled mode Sep 26, 2018

kuzhelov added the 🚧 WIP label Sep 26, 2018

kuzhelov-ms added 3 commits September 26, 2018 23:46

Merge branch 'master' into feat/refactor-popup-ex

f8c8c01

add keys for example components array

a6f6e78

Merge branch 'master' into feat/refactor-popup

8862d39

kuzhelov added 🚀 ready for review and removed 🚧 WIP labels Sep 26, 2018

sophieH29 reviewed Sep 27, 2018

View reviewed changes