Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Popup): Multiple event triggering a Popup #1977

Merged
merged 9 commits into from
Aug 20, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/app/Examples/modules/Popup/Usage/PopupExampleClick.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const PopupExampleClick = () => (
<Popup
trigger={<Button color='red' icon='flask' content='Activate doomsday device' />}
content={<Button color='green' content='Confirm the launch' />}
on='click'
on={['click']}
Copy link
Member

Choose a reason for hiding this comment

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

Let's revert this

position='top right'
/>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class PopupExampleControlled extends React.Component {
<Popup
trigger={<Button content='Open controlled popup' />}
content={`This message will self-destruct in ${timeoutLength / 1000} seconds!`}
on='click'
on={['click']}
open={this.state.isOpen}
onClose={this.handleClose}
onOpen={this.handleOpen}
Expand Down
2 changes: 1 addition & 1 deletion docs/app/Examples/modules/Popup/Usage/PopupExampleFocus.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const PopupExampleFocus = () => (
trigger={<Input icon='search' placeholder='Search...' />}
header='Movie Search'
content='You may search by genre, header, year and actors'
on='focus'
on={['focus']}
/>
)

Expand Down
2 changes: 1 addition & 1 deletion docs/app/Examples/modules/Popup/Usage/PopupExampleHover.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const PopupExampleHover = () => (
<Popup
trigger={<Button icon='add' content='Add a friend' />}
content='Sends an email invite to a friend.'
on='hover'
on={['hover']}
/>
)

Expand Down
13 changes: 13 additions & 0 deletions docs/app/Examples/modules/Popup/Usage/PopupExampleMultiple.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react'
import { Input, Popup } from 'semantic-ui-react'

const PopupExampleMultiple = () => (
<Popup
trigger={<Input icon='search' placeholder='Search...' />}
header='Movie Search'
content='You may search by genre, header, year and actors'
on={['focus', 'hover']}
/>
)

export default PopupExampleMultiple
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react'
import { Button, Grid, Popup } from 'semantic-ui-react'

const PopupExampleNested = () => (
<Popup wide trigger={<Button content='Are you the one?' />} on='click'>
<Popup wide trigger={<Button content='Are you the one?' />} on={['click']}>
<Grid divided columns='equal'>
<Grid.Column>
<Popup
Expand Down
5 changes: 5 additions & 0 deletions docs/app/Examples/modules/Popup/Usage/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ const PopupUsageExamples = () => (
description='A popup can be nested inside another.'
examplePath='modules/Popup/Usage/PopupExampleNested'
/>
<ComponentExample
title='Multiple'
description='A popup can be triggered on multiple events.'
examplePath='modules/Popup/Usage/PopupExampleMultiple'
/>
<ComponentExample
title='Controlled'
description='A popup can have its visibility controlled from outside.'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const PopupExampleHideOnScroll = () => (
<Popup
trigger={<Button icon>Click me</Button>}
content='Hide the popup on any scroll event'
on='click'
on={['click']}
hideOnScroll
/>
<Popup
Expand Down
2 changes: 1 addition & 1 deletion src/modules/Popup/Popup.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export interface PopupProps extends PortalProps {
offset?: number;

/** Event triggering the popup. */
on?: 'hover' | 'click' | 'focus';
on?: Array<string>;
Copy link
Member

Choose a reason for hiding this comment

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

'hover' | 'click' | 'focus' | Array<'hover' | 'click' | 'focus'>
This should be correct as I remember


/**
* Called when a close event happens.
Expand Down
13 changes: 7 additions & 6 deletions src/modules/Popup/Popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export default class Popup extends Component {
offset: PropTypes.number,

/** Event triggering the popup. */
on: PropTypes.oneOf(['hover', 'click', 'focus']),
on: PropTypes.arrayOf(PropTypes.oneOf(['hover', 'click', 'focus'])),
Copy link
Member

Choose a reason for hiding this comment

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

Valid propType will be:

propTypes.oneOfType([
  PropTypes.oneOf(['hover', 'click', 'focus']),
  PropTypes.arrayOf(['hover', 'click', 'focus']), 
])


/**
* Called when a close event happens.
Expand Down Expand Up @@ -127,7 +127,7 @@ export default class Popup extends Component {

static defaultProps = {
position: 'top left',
on: 'hover',
on: ['hover'],
Copy link
Member

Choose a reason for hiding this comment

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

We can left this

}

static _meta = {
Expand Down Expand Up @@ -250,15 +250,16 @@ export default class Popup extends Component {
portalProps.closeOnPortalMouseLeave = true
portalProps.mouseLeaveDelay = 300
}

if (on === 'click') {
if (_.indexOf(on, 'click') > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

_.includes can be used there

Copy link
Member

Choose a reason for hiding this comment

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

We can normalize value there and then use it:

const normalizedOn = _.isArray(on) ? on : [on]

This will allow to pass on='hover' and on={['hover']}

portalProps.openOnTriggerClick = true
portalProps.closeOnTriggerClick = true
portalProps.closeOnDocumentClick = true
} else if (on === 'focus') {
}
if (_.indexOf(on, 'focus') > -1) {
portalProps.openOnTriggerFocus = true
portalProps.closeOnTriggerBlur = true
} else if (on === 'hover') {
}
if (_.indexOf(on, 'hover') > -1) {
portalProps.openOnTriggerMouseEnter = true
portalProps.closeOnTriggerMouseLeave = true
// Taken from SUI: https://git.io/vPmCm
Expand Down
8 changes: 4 additions & 4 deletions test/specs/modules/Popup/Popup-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ describe('Popup', () => {
content='_'
position={position}
trigger={<button>foo</button>}
on='click'
on={['click']}
/>,
)
wrapper.find('button').simulate('click')
Expand All @@ -134,7 +134,7 @@ describe('Popup', () => {
content='_'
position={position}
trigger={<button>foo</button>}
on='click'
on={['click']}
offset={999}
/>,
)
Expand Down Expand Up @@ -176,7 +176,7 @@ describe('Popup', () => {
describe('trigger', () => {
it('it appears on click', () => {
const trigger = <button>foo</button>
wrapperMount(<Popup on='click' content='foo' header='bar' trigger={trigger} />)
wrapperMount(<Popup on={['click']} content='foo' header='bar' trigger={trigger} />)

wrapper.find('button').simulate('click')
assertInBody('.ui.popup.visible')
Expand All @@ -195,7 +195,7 @@ describe('Popup', () => {

it('it appears on focus', () => {
const trigger = <input type='text' />
wrapperMount(<Popup on='focus' content='foo' trigger={trigger} />)
wrapperMount(<Popup on={['focus']} content='foo' trigger={trigger} />)

wrapper.find('input').simulate('focus')
assertInBody('.ui.popup.visible')
Expand Down