Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

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

Merged
merged 20 commits into from
Oct 1, 2018
Merged

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Sep 26, 2018

TODO


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

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

codecov bot commented Sep 26, 2018

Codecov Report

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

Impacted file tree graph

@@           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.

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

class PopupArrowExample extends React.Component<any, any> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the name is PopupArrow? :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 name should reflect name of file

/** The popup content (deprecated). */
children: customPropTypes.disallow(['children']),
children: PropTypes.any, // customPropTypes.disallow(['children']),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kuzhelov kuzhelov Sep 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any thoughts here?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls take a look at comments

style={popupPlacementStyles}
{...accessibility.attributes.popup}
{...accessibility.keyHandlers.popup}
data-placement={placement}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kuzhelov kuzhelov Sep 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@bmdalex bmdalex Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking about line 162:

data-placement={placement}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, got it - let me address..

{ 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 = () => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👍

@mnajdova
Copy link
Contributor

mnajdova commented Oct 1, 2018

I think there is some issue with the RTL. Please take a look into the gif:
ezgif com-gif-maker 1

@kuzhelov
Copy link
Contributor Author

kuzhelov commented Oct 1, 2018

I think there is some issue with the RTL. Please take a look into the gif:
ezgif com-gif-maker 1

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

Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved

@kuzhelov kuzhelov merged commit 2a3b3e6 into master Oct 1, 2018
@kuzhelov kuzhelov deleted the feat/refactor-popup branch November 19, 2018 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants