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

vipul-21 · 2017-08-17T09:41:57Z

Popup triggering by multiple events(hover, focus or click)

The position of the Popup on elements with smaller width(<25px) not aligned.

Popup triggering by multiple events(hover, focus or click)

codecov-io · 2017-08-17T10:11:31Z

Codecov Report

Merging #1977 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1977      +/-   ##
=========================================
+ Coverage    99.8%   99.8%   +<.01%     
=========================================
  Files         148     148              
  Lines        2568    2569       +1     
=========================================
+ Hits         2563    2564       +1     
  Misses          5       5

Impacted Files	Coverage Δ
src/modules/Popup/Popup.js	`100% <100%> (ø)`	⬆️

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 a4780cd...1354c3b. Read the comment docs.

layershifter

@vipul-21 Thanks for PR! But we can't merge it as is because it will introduce a breaking change. However, we can easily omit it, let's use normalization of on, this will allow to left compability with current API.

layershifter · 2017-08-18T07:21:36Z

src/modules/Popup/Popup.js

@@ -250,15 +250,16 @@ export default class Popup extends Component {
      portalProps.closeOnPortalMouseLeave = true
      portalProps.mouseLeaveDelay = 300
    }
-
-    if (on === 'click') {
+    if (_.indexOf(on, 'click') > -1) {


_.includes can be used there

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']}

layershifter · 2017-08-18T07:21:56Z

src/modules/Popup/Popup.js

@@ -127,7 +127,7 @@ export default class Popup extends Component {

  static defaultProps = {
    position: 'top left',
-    on: 'hover',
+    on: ['hover'],


We can left this

layershifter · 2017-08-18T07:22:53Z

src/modules/Popup/Popup.js

@@ -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'])),


Valid propType will be:

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

layershifter · 2017-08-18T07:23:32Z

src/modules/Popup/Popup.d.ts

@@ -39,7 +39,7 @@ export interface PopupProps extends PortalProps {
  offset?: number;

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


layershifter · 2017-08-18T07:23:44Z

docs/app/Examples/modules/Popup/Usage/PopupExampleClick.js

@@ -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']}


Let's revert this

vipul-21 · 2017-08-18T09:21:44Z

@layershifter I have updated the changes as recommended. But the array being assigned to on is giving a type error. I am not able to figure out why. Can you help me out ?

layershifter · 2017-08-18T09:51:47Z

docs/app/Examples/modules/Popup/Usage/PopupExampleClick.js

@@ -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'}


Just on='click', please

layershifter · 2017-08-18T09:51:53Z

docs/app/Examples/modules/Popup/Usage/PopupExampleControlled.js

@@ -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'}


layershifter · 2017-08-18T09:51:59Z

docs/app/Examples/modules/Popup/Usage/PopupExampleFocus.js

@@ -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'}


layershifter · 2017-08-18T10:05:30Z

You're right, PropTypes.arrayOf accepts type, not value, so correct propType will be PropTypes.arrayOf(PropTypes.oneOf(['hover', 'click', 'focus'])),

layershifter · 2017-08-18T10:10:18Z

@vipul-21 Thanks for changes, we need only to add test

describe('trigger', () => {
    //  .....


    it('it appears on multiple', (done) => {
      const trigger = <button>foo</button>
      const button = wrapperMount(<Popup on={['click', 'hover']} content='foo' header='bar' trigger={trigger} />)
        .find('button')

      button.simulate('click')
      assertInBody('.ui.popup.visible')

      domEvent.click('body')

      button.simulate('mouseenter')
      setTimeout(() => {
        assertInBody('.ui.popup.visible')
        done()
      }, 51)
    })
  })

levithomason · 2017-08-20T17:37:17Z

Very cool, thanks

levithomason · 2017-08-20T20:58:44Z

Released in semantic-ui-react@0.71.5

vipul added 4 commits August 16, 2017 20:15

fix(Popup): change position(right/left) for small width components

1cab40a

The position of the Popup on elements with smaller width(<25px) not aligned.

Review changes updated

917297a

feat(Popup): Multiple event triggering a Popup

de6518d

Popup triggering by multiple events(hover, focus or click)

test case for proptypes

d8ef3b3

layershifter requested changes Aug 18, 2017

View reviewed changes

layershifter added enhancement needs author feedback labels Aug 18, 2017

Requested changes updated

f799471

layershifter reviewed Aug 18, 2017

View reviewed changes

vipul added 2 commits August 18, 2017 15:23

Proptypes validation for Popup

6a8227f

Review changes updated

883818f

vipul added 2 commits August 18, 2017 15:46

Test case for multiple event trigger Popup

b0c4c67

Rectifying indentation

1354c3b

layershifter added ready for review and removed needs author feedback labels Aug 18, 2017

layershifter approved these changes Aug 18, 2017

View reviewed changes

levithomason merged commit 87bae70 into Semantic-Org:master Aug 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vipul-21 commented Aug 17, 2017

codecov-io commented Aug 17, 2017 •

edited

Loading

layershifter left a comment

layershifter Aug 18, 2017

layershifter Aug 18, 2017

layershifter Aug 18, 2017

layershifter Aug 18, 2017

layershifter Aug 18, 2017

layershifter Aug 18, 2017

vipul-21 commented Aug 18, 2017 •

edited

Loading

layershifter Aug 18, 2017

layershifter Aug 18, 2017

layershifter Aug 18, 2017

layershifter commented Aug 18, 2017 •

edited

Loading

layershifter commented Aug 18, 2017

levithomason commented Aug 20, 2017

levithomason commented Aug 20, 2017

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

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

Conversation

vipul-21 commented Aug 17, 2017

codecov-io commented Aug 17, 2017 • edited Loading

Codecov Report

layershifter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vipul-21 commented Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Aug 18, 2017 • edited Loading

layershifter commented Aug 18, 2017

levithomason commented Aug 20, 2017

levithomason commented Aug 20, 2017

codecov-io commented Aug 17, 2017 •

edited

Loading

vipul-21 commented Aug 18, 2017 •

edited

Loading

layershifter commented Aug 18, 2017 •

edited

Loading