feat(Popup): add component #570

typehorror · 2016-09-30T01:34:08Z

Port of SUI Popup module

Fixes #193

To be done later:

inline (popup as a sibling of trigger, will be implemented later)
transition (animation will probably be for later)

codecov-io · 2016-09-30T01:39:34Z

Current coverage is 100% (diff: 100%)

Merging #570 into master will not change coverage

@@           master   #570   diff @@
====================================
  Files         119    122     +3   
  Lines        1890   2000   +110   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         1890   2000   +110   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update 978f206...9a5df1c

typehorror · 2016-09-30T04:48:08Z

Hey @levithomason,

I know that's not your initial design but I feel like adding an anchor element and an open state would simplify the implementation and open the way to additional feature (like a click trigger instead of a hover). The counter argument would be that it would complicate usage, as the user would have to maintain a state for the open status. The anchor element could be transmitted using event.currentTarget by the event handler.

Something among those lines:

export default class PopupOnClick extends Component {
  buttonClick = (event) =>
    this.setState({
      open: true,
      target: event.currentTarget,
    })

  render() {
    return <div>
      <button onClick={this.buttonClick} >Click me</button>
      <Popup 
        content="foo bar"
        open={this.state.open}
        target={this.state.target}
      />
    </div>
  }
}

I don't want to impose my style, I came here to help and I'm totally open to your opinion.

layershifter · 2016-09-30T06:00:15Z

docs/app/Examples/modules/Popup/Types/PopupExample.js

+import React, { Component } from 'react'
+import { Button, Popup } from 'stardust'
+
+export default class PopupExample extends Component {


We can keep example stateless there.

I'll keep in mind to use stateless function when no state is required 👍

layershifter · 2016-09-30T06:07:08Z

src/modules/Popup/Popup.js

+    )
+
+    this.setState({ popup })
+  }


Popup will be rerendered on each show.

layershifter · 2016-09-30T06:17:31Z

I think we can transform render part to this.

const { active } = this.state

return (
   <div>
     <Portal isOpened={active}>{popup}</Portal>
     {trigger}
  </div>
 )

Also, I like SUI ideas there, can we implement them there?

typehorror · 2016-09-30T07:05:14Z

I've made a few updates to the handling part of the positioning without introducing refs but I had to stop having the popup rendering the trigger.

While having an open prop on the popover isn't exactly the SUI approach, I believe it's actually closer to the react way. @layershifter this also opens the door to custom event triggering for the popup as you suggested (see my examples demonstrating onFocus, onMouseOver and onClick)

jeffcarbs · 2016-09-30T07:07:47Z

Hey so I wound up writing our own Portal component (based on react-portal which we're currently using). I explain in more detail why in the comment here https://github.com/TechnologyAdvice/stardust/pull/571, but I think the changes could be useful here:

It takes a trigger element which let's you avoid the wrapping div.
You can optionally configure to show up onClick or onMouseOver.
Control it yourself or let the open state be handled by the component.
Currently doesn't support closing on mouseleave but I figured we could add that type of functionality on this PR given the modal really doesn't have that use-case.

If you have any feedback on some of the open items on that issue I'd appreciate it. Assuming we like that approach, the sooner we can work out the kinks the sooner we can apply it here.

typehorror · 2016-09-30T07:17:54Z

@jcarbo this could indeed resolve my initial problem. I would need an additional onMouseLeave on your implementation but it looks like it would be a trivial addition (unless I missed it). Lastly I think your Portal version would allow me to retrieve the event and therefore position the popover accurately.

PS: I believe there could be value in having the popup on focus (help on an input field), so maybe adding onFocus and onBlur event could be nice to have.

jeffcarbs · 2016-09-30T07:46:56Z

@Debrice - thanks for the suggestions! Wound up implementing all three options (closing onMouseLeave/onBlur and opening onFocus), see TechnologyAdvice/stardust@92390a0 and TechnologyAdvice/stardust@e92914a.

typehorror · 2016-09-30T14:43:05Z

This is great, thank you @jcarbo , I'll pull your branch in and start implementing today.

levithomason

I know this is a WIP, here are few comments along the way. Thanks much for getting this going!

levithomason · 2016-09-30T23:37:16Z

docs/app/Examples/modules/Popup/Triggers/PopupClickExample.js

+import React from 'react'
+import { Button, Icon, Popup } from 'stardust'
+
+export default function () {


In light of #564, let's name example classes/functions PopupExampleClick. Both the filename and default export should have the same name as well.

levithomason · 2016-09-30T23:40:15Z

docs/app/Examples/modules/Popup/Triggers/PopupClickExample.js

+    <Button.Group>
+      <Button color='red'>Activate doomsday device</Button>
+      <Popup
+        trigger={<Button color='black' icon><Icon name='question' /></Button>}


The button can be simplified with shorthand syntax here. Shorthand is very useful for cases exactly like this, passing components through props:

trigger={<Button color='black' icon='question' />}

Then, you can also remove the Icon import.

levithomason · 2016-09-30T23:44:10Z

docs/app/Examples/modules/Popup/Types/PopupHtmlExample.js

+import React from 'react'
+import { Popup, Card, Rating, Image } from 'stardust'
+
+const users = [


Unused var here, npm run lint to see lint issues.

I use the linter (also in my editor), all those variables are being used.

Ah, I believe I'm getting confused on the new GH review flow. This comment thread does not show it in use for some reason.

https://github.com/debrice/stardust/blob/d51317361c15723d2796c85a1752ee4d5fc63584/docs/app/Examples/modules/Popup/Types/PopupHtmlExample.js

And you are right, I'm so confused 😞 It was a left over from a copy/paste and indeed my linter catch it too. I'm trying to do too many things at the same time.

Yep, been there :) Onward!

levithomason · 2016-09-30T23:49:13Z

docs/app/Examples/modules/Popup/Types/PopupHtmlExample.js

+    trigger={IndividualCard}
+    content={content}
+    hover
+  />


Few points on this one:

Let's use children for defining full markup

Return multi-line JSX inside of parens (npm run lint to see all issues)

Use sub components for SUI component parts. See this section in the CONTRIBUTING.md for more details on API design.

Given our design and style patterns noted here ^, this example would look more like this:

return ( <Popup trigger={IndividualCard} hover> <Popup.Header>User Rating</Popup.Header> <Popup.Content> <Rating icon='star' defaultRating={3} maxRating={4} /> </Popup.Content> </Popup> )

Note, I also realize now that there is a name conflict between the data-title prop and the class="header" component part. Let's go with the Popup class names (header, etc) for our prop names and sub component names.

sounds good, I was also confused about title and header and was about to ask for confirmation 👍

Somehow the linter does not complain about those being multi-line.
Great, going to implement those sub rendered.

Hm, I get this in my editor:

And this from the CLI:

Maybe double check the linter setup and that the latest modules are installed? I know NPM has had some issues as of late, sometimes requiring multiple installs to get all the modules you should've gotten the first go. LMK if you have further issues and we can investigate deeper.

levithomason · 2016-09-30T23:55:50Z

docs/app/Examples/modules/Popup/Variations/PopupWidthExample.js

+    <div>
+      <Popup
+        trigger={<Icon circular name='heart' />}
+        content='Hello. This is a wide pop-up which allows for lots of content


Single quotes can't handle line breaks. Though we could use backticks to preserve the line breaks, I think we should instead use children here:

<Popup trigger={<Icon circular name='heart' />} width='wide' hover> Hello. This is a very wide pop-up which allows for lots of content with additional space. You can fit a lot of words here and the paragraphs will be pretty wide. </Popup>

I somehow believed you were against using props.children, my bad.

No worries. Our goal is to provide two APIs 1) a "Shorthand Props API" for the majority simplified use cases and 2) a sub component API as an escape hatch when you need access and control over the full markup.

It is worth noting, the two are mutually exclusive. So, you cannot mix a shorthand header prop while also having children defined. More here in the CONTRIBUTING.md. Also, I plan to add more info to the docs on this, #561.

Here are some equivalent examples:

<Popup content='The content' /> <Popup> The content <Popup>

<Popup header='My Header' content='The content' /> <Popup> <Popup.Header>My header</Popup.Header> <Popup.Content>The content</Popup.Content> <Popup>

Most components have shorthand capabilities you can reference for more examples.

levithomason · 2016-09-30T23:56:46Z

src/modules/Popup/Popup.js

@@ -1,7 +1,8 @@
 import React, { Component, PropTypes } from 'react'
 import { findDOMNode } from 'react-dom'
 import { META } from '../../lib'
-import Portal from 'react-portal'
+import Portal from '../../addons/Portal/Portal'
+import classnames from 'classnames'


We're using cx as the import name for classnames. Let's update for consistency.

levithomason · 2016-10-01T00:00:06Z

src/modules/Popup/Popup.js

-    const positions = (this.props.position || 'top left').split(' ')
+    if (!coords) return
+
+    const positions = this.props.position.split(' ')
    const style = { position: 'absolute' }

    if (positions.includes('right')) {


includes has limited browser support. Let's use _.includes(positions, 'right') instead here.

levithomason · 2016-10-01T00:01:51Z

Left an interim review for now. LMK when it's stable enough for a full review and I'll pull it down, try it out, and offer more input. Cheers!

typehorror · 2016-10-01T01:28:24Z

@levithomason I'm at a place where I could use some feedback from a UI experience. I'm not familiar with animation for example and how you expect them to be handled.

Also I'm using findDOMNode which I believe isn't a sustainable solution but I should be able to drop it once I get access to the trigger's event from Portal.

Positioning will need to have the "center" attribute too, but I can't think of a way to read the size of the popup without using ref with a function (...or react-dom):

//...
  popupMounted = (ref) => {
    this.setState({'popupBoundingRect': ref.getBoundingClientRect()})
  }
  render(){
    return <Popover ref={this.popupMounted} />
  }
}

Thank you for your help and patience!

jeffcarbs · 2016-10-01T04:46:18Z

src/modules/Popup/Popup.js

+import { META } from '../../lib'
+import Portal from '../../addons/Portal/Portal'
+import cx from 'classnames'
+import isString from 'lodash/isString'


The standard pattern in this project has been to include lodash as _ as the very first import within the file. We use the babel-plugin-lodash plugin which handles cherry-picking only the functions you use (see https://github.com/lodash/babel-plugin-lodash#example)

Great, I got into the habit of cherry picking method of the any library for size concerns. I was wondering if the same applies to stardust element then? If someone only wanted to import the Popup functionality, then if this person was using:

import Popup from 'stardust/modules/Popup'

the way we import Portal in Popup:

import Portal from '../../addons/Portal/Portal'

instead of

import { Portal } from '../../addons'

wouldn't it impact the size of the import by loading the whole addons content ?

Indeed, let's use the direct filename import. I've commented in #524 to reflect this issue.

levithomason · 2016-10-01T05:50:19Z

I'm at a place where I could use some feedback from a UI experience. I'm not familiar with animation for example and how you expect them to be handled.

We're forgoing animations until we get all components and APIs done. We are just adding/removing the visibility classes as needed for now. Later, we'll add a utility that reuses SUI CSS transitions by adding these classes (and styles) in sequence to trigger animations. See #200 for more notes on that.

Also I'm using findDOMNode which I believe isn't a sustainable solution but I should be able to drop it once I get access to the trigger's event from Portal.

👍 Looks like the Portal is now passing the necessary events, see https://github.com/TechnologyAdvice/stardust/pull/571#issuecomment-250891167

Positioning will need to have the "center" attribute too, but I can't think of a way to read the size of the popup without using ref with a function (...or react-dom):

Measuring is one of those things (focus being the other) that you just have to use a ref for. I think you're OK there. Though perhaps we consider borrowing a pattern from something like react-measure. Where we have an abstract component that can measure other components. I don't want to use react-measure directly since it has a dependency element-resize-detector that is >1600 LOC.

levithomason

Left another round of comments for you. Thanks for putting up with it! I'll try to focus on getting the Portal merged before doing a final here. That way this PR only includes the Popup work.

levithomason · 2016-10-01T05:55:51Z

src/modules/Popup/Popup.js

+    fluid: PropTypes.bool,
+
+    /** Display the popup on trigger focus */
+    focus: PropTypes.bool,


Propose we pull these into an on enum prop to keep in line with SUI, but also avoid the confusion of focus={true} not actually setting focus, but setting the event to open on.

<Popup on='focus' /> <Popup on='hover' /> <Popup on='click' />

levithomason · 2016-10-01T05:57:48Z

src/modules/Popup/Popup.js

+}
+
+/**
+ * A <Popup /> displays additional information on top of a page.


Let's stick with the plain text name, Popup, here to be consistent with other component descriptions.

levithomason · 2016-10-01T06:00:22Z

src/modules/Popup/Popup.js

+    header: PropTypes.string,
+
+    /** Display the popup on mouse over the trigger */
+    hover: PropTypes.bool,


When this prop is pulled into the on enum prop, could we add hoverable here?

levithomason · 2016-10-01T06:19:49Z

src/modules/Popup/Popup.js

+    const { header, content } = this.props
+
+    if (header) {
+      children.push(<PopupHeader key={index++}>{header}</PopupHeader>)


This method can go away in favor of shorthand factories suggested in the main render function. However, for future reference avoid using index in keys. We have a few in the codebase still that we're removing. It is an anti pattern that breaks React's ability to track/update children.

levithomason · 2016-10-01T06:25:51Z

src/modules/Popup/Popup.js

+      width,
+    } = this.props
+
+    const { coords } = this.state


Let's add portalProps used below as well. I should add some lint rules for this since we destructure props/state always.

levithomason · 2016-10-01T06:26:23Z

src/modules/Popup/Popup.js

+    return (
+      <Portal trigger={trigger} {...this.state.portalProps}>
+        <div
+          style={this.getPosition(coords)}


Suggest renaming to this.getStyle() given the usage.

levithomason · 2016-10-01T06:38:09Z

src/modules/Popup/Popup.js

+          style={this.getPosition(coords)}
+          className={classes}
+        >
+        { children || this.renderContent() }


Pending adding shorthand create() methods to the sub components, this.renderChildren() here can be replaced with this:

render() { return ( <Portal trigger={trigger} {...portalProps}> <div style={this.getStyle(coords)} className={classes}> {children} {!children && PopupHeader.create(header)} {!children && PopupContent.create(content)} </div> </Portal> ) }

The create() shorthand can be created by importing createShorthandFactory() and adding a static method to each component:

import { createShorthandFactory } from '../../lib' // PopupHeader.js PopupHeader.create = createShorthandFactory(PopupHeader, value => ({ children: value })) // PopupContent.js PopupContent.create = createShorthandFactory(PopupContent, value => ({ children: value }))

The second argument is the mapValueToProps() function. When PopupHeader.create('my header') is called, that function will map the string to children. Factories also know how to handle numbers, props objects, and elements as a value. So, your props will be able to handle many types of data. You can read more in factories.js and see it in other components. Again, more docs on the way for this, #561!

levithomason · 2016-10-01T06:38:42Z

src/modules/Popup/PopupContent.js

+import { META } from '../../lib'
+
+/**
+ * A <PopupContent /> displays the content body of a Popover.


For consistency <PopupContent /> => PopupContent

levithomason · 2016-10-01T06:40:14Z

src/modules/Popup/PopupContent.js

+}
+
+PopupContent.propTypes = {
+  /** The header of the Popup */


Copy pasta? Probably should be content instead of header.

levithomason · 2016-10-01T06:40:26Z

src/modules/Popup/PopupHeader.js

+import { META } from '../../lib'
+
+/**
+ * A <PopupHeader /> displays a header in a Popover.


<PopupHeader /> => PopupHeader

levithomason · 2016-10-01T06:44:06Z

src/modules/Popup/Popup.js

+import React, { Component, PropTypes } from 'react'
+import { findDOMNode } from 'react-dom'
+import { META } from '../../lib'
+import Portal from '../../addons/Portal/Portal'


Doesn't require a change, but FYI you can import components as named exports from the category folder index if you prefer:

import { Portal } from '../../addons'

Especially handy when there are several imports:

import { Button, Icon, Label } from '../../elements' // vs import Button from '../../elements/Button/Button' import Icon from '../../elements/Icon/Icon' import Label from '../../elements/Label/Label'

in the case of the second example vs the first, wouldn't that impact the size of the final bundle if someone was to cherry pick a single component for a project ?

import Button from 'stardust/elements/Button'

Indeed you are correct. I hadn't thought that through... It would automatically bundle all "elements". We're targeting small custom builds as part of our goals, #524. So from now on let's stick with directly importing files. Thanks for pointing it out.

jeffcarbs · 2016-10-02T03:58:59Z

@Debrice - could you rebase this PR against the most recent master? The Portal code has been merged in so it'll make the changes easier to read.

typehorror · 2016-10-02T18:03:52Z

@jcarbo I've never rebased any code in the past, so that was quite an adventure (100 commits now).

typehorror · 2016-10-02T19:10:02Z

@levithomason @jcarbo I'm a bit unfamiliar with rebasing/cleaning up a PR . I believe I have 2 options now:

I can have a single commit on my fork master branch but I would have to create a new PR.
I could squash the commits on my current branch and force push the single squash commit.

Any advice ?

jeffcarbs · 2016-10-02T19:28:13Z

I'd squash down the current branch to one commit. So assuming 2c144d1 is the commit you're rebased off of you would do:

git reset 2c144d1 - would put you back at 2c144d1 with all of the changes from this branch unstaged
git add . - stage your changes
git commit -m 'feat(Popup): add component'
git push --force

All commits on a feature branch wind up getting squashed down to one anyway when they're merged into master but this would just clean things up a bit for now.

typehorror · 2016-10-02T20:45:54Z

@jcarbo thanks for the help, force push is done 🎊

Regarding Portal, I need to build hoverable props, which would allow a user to hover from the trigger to the popup, without the popup going away by the mouseleave:

Should that be a feature of the Portal or the Popup? For it to be on the Popup, I believe I would still need some changes on the Portal to grant more control on the event.

levithomason · 2016-10-03T00:14:02Z

Regarding the hoverable prop, I'd think that would set the Portal closeOnMouseLeave to false but keep openOnMouseEnter set to true.

This would leave the Popup open with no way to close it, since blur (mouseLeave) doesn't. Perhaps we'd use some kind of timer that would close the Popup but reset on hover. This way the user can move the mouse off the trigger (timer starts), mouse enters the Popup (timer cancelled), mouse leaves Popup (timer starts again), finally after X seconds the timer expires and the Popup closes.

jeffcarbs · 2016-10-03T01:02:50Z

I think we can handle it by modifying one prop and adding another:

Change closeOnTriggerMouseLeave to just closeOnMouseLeave. Calls onClose when the mouse leaves the trigger OR the portal (currently it's just the trigger).
Add mouseLeaveDelay. On mouseleave, we delay for mouseLeaveDelay milliseconds before calling onClose and if we receive a mouseenter on the trigger OR portal during the delay we clear the timer. This is actually what SUI proper does. We could default to 70ms like they do (https://github.com/Semantic-Org/UI-Popup/blob/master/popup.js#L1373-L1377).

I can also see adding two other props for symmetry:

closeOnTriggerClick
mouseEnterDelay - SUI defaults this to 50ms

I'll open up a quick PR with these additions unless there are any objections.

levithomason · 2016-10-04T18:38:26Z

I've already migrated to the new index.js structure

Indeed, my branch was old, apologies!

@jcarbo agreed, let's feature freeze this where it is, cleanup and merge. Then any extras can come later. I'll start a final review now.

typehorror · 2016-10-04T18:40:03Z

@jcarbo agreed, let's feature freeze this where it is, cleanup and merge. Then any extras can come later. I'll start a final review now.

@levithomason in that case I'll work on spec ASAP

levithomason

If any of these comments seem like adding features, let's comment out the feature with a TODO and push them off to another PR. This way we can break down this work into smaller pieces 👍

levithomason · 2016-10-04T18:39:50Z

docs/app/Examples/modules/Popup/Triggers/PopupClickExample.js

+import React from 'react'
+import { Button, Popup } from 'semantic-ui-react'
+
+export default function PopupClickExample() {


In keeping with other exmples, let's update the example syntax in this PR from:

export default function PopupClickExample() { return ( // ... ) }

To:

export default const PopupClickExample = () => ( // ... )

levithomason · 2016-10-04T18:42:00Z

docs/app/Examples/modules/Popup/Triggers/PopupFocusExample.js

+import React from 'react'
+import { Input, Popup } from 'semantic-ui-react'
+
+export default function PopupFocusExample() {


Commenting here, but for all example filenames and default export names. Let's use the latest pattern PopupExample___.:

PopupExampleFocus.js export default PopupExampleFocus PopupExampleHideOnScroll.js export default PopupExampleHideOnScroll ...

levithomason · 2016-10-04T18:43:30Z

docs/app/Examples/modules/Popup/Triggers/PopupHoverExample.js

+  return (
+    <div>
+      <Popup
+        trigger={<Button><Icon name='add' />Add a friend</Button>}


Prefer shorthand icon button here for brevity and less imports (no Icon import required):

trigger={<Button icon='add' content='Add a friend' />}

levithomason · 2016-10-04T19:04:47Z

docs/app/Examples/modules/Popup/Triggers/index.js

+import ExampleSection from 'docs/app/Components/ComponentDoc/ExampleSection'
+
+export default function () {
+  return (


All files' default export should match the filename. Since index.js files don't make sense in this pattern, we're using the "Component + Directory + Examples". Also note the const vs function, let's update to use:

const PopupTriggersExamples = () => ( // ... ) export default PopupTriggersExamples

levithomason · 2016-10-04T19:05:30Z

docs/app/Examples/modules/Popup/Types/PopupExample.js

+  return (
+    <div>
+      <Popup
+        trigger={<Button icon><Icon name='add' /></Button>}


Please use shorthand icon button:

<Button icon='add' />

levithomason · 2016-10-04T19:23:04Z

src/modules/Popup/Popup.js

+    flowing: PropTypes.bool,
+
+    /** Takes up the entire width of its offset container */
+    fluid: PropTypes.bool,


Since this example is not working, let's comment this out with a TODO.

levithomason · 2016-10-04T19:28:09Z

src/modules/Popup/Popup.js

+    trigger: PropTypes.node,
+
+    /** Popup width */
+    width: PropTypes.string,


There are fixed widths, though I think we should update this to wide and wide='very' given the SUI classes at hand.

_meta = { props: { wide: ['very'] }, } wide: customPropTypes.some([ PropTypes.bool, PropTypes.oneOf(_meta.props.wide) ]),

Here's more context as to why the rename. The width is primarily used for components that have grid column style widths, or a group component that defines the widths of its children. It takes a number or a word (width={4} width='four'). In this case, there it is more like the padded or relaxed prop, where it is either boolean or takes a very value.

levithomason · 2016-10-04T19:33:15Z

docs/app/Examples/modules/Popup/Variations/PopupOffsetExample.js

+export default function PopupOffsetExample() {
+  return (
+    <div>
+      <Popup


We can remove the extra div wrapper here and just return the <Popup />. As long as a single element is returned, React is happy. React only requires an extra containing element (such as a div) if multiple siblings need to be returned.

levithomason · 2016-10-04T19:42:22Z

src/modules/Popup/Popup.js

+        style.right -= offset
+      } else {
+        style.left += offset
+      }


Expected Behavior

This logic seems to always offset to the right. I'd expect the offset to be in the opposite direction of the position:

position='top center' // offset upward position='left center' // offset to the left position='bottom center' // offset downward

Or, provide an offset object with +/- horizontal and vertical values that would be applied:

position='top center' // offset upward offset={{ vertical: -5 }} position='top center' // offset downward offset={{ vertical: 5 }}

Current Behavior

position='top center'

position='left center'

position='bottom center'

I thought of the same thing, but it seems like SUI offset is only acting on the X axis and from left to right. I can change to one of your suggestion if you want me to

levithomason · 2016-10-04T19:46:39Z

src/modules/Popup/Popup.js

+
+    return (
+      <Portal trigger={trigger} {...this.getPortalProps()}>
+        <div ref={this.popupMounted} style={style} className={classes} >


Extra space here before closing angle: className={classes} >

layershifter · 2016-10-04T20:10:45Z

src/modules/Popup/Popup.js

+
+    return (
+      <Portal trigger={trigger} {...this.getPortalProps()}>
+        <div ref={this.popupMounted} style={style} className={classes} >


I think there might be ElementType from getElementType instead of div.

const ElementType = getElementType(Popup, props) <ElementType {...rest} ...>...</ElementType>

layershifter · 2016-10-04T20:15:23Z

src/modules/Popup/PopupContent.js

+ * A PopupContent displays the content body of a Popover.
+ */
+export default function PopupContent(props) {
+  return <div className='content'>{ props.children }</div>


Use there classnames, ElementType and getUnhandledProps.

const { children, className } = props const classes = cx('content', className) const rest = getUnhandledProps(PopupContent, props) const ElementType = getElementType(PopupContent, props) return <ElementType {...rest} className={classes}>{children}</ElementType>

Again, nice catch. Thanks!

Thanks for the guidance guys, your reviews are super helpful! 👍

typehorror · 2016-10-05T00:19:29Z

@levithomason, @layershifter I addressed the PR comments, will start working on the specs now.

Also @jcarbo I can't get the focus through click to behave on the trigger (the popup appears and disappear almost instantly). Would you mind checking that my Portal config https://github.com/TechnologyAdvice/stardust/pull/570/files#diff-daca312d5932d8a0a079b86773c0ee3cR206 ?

jeffcarbs · 2016-10-05T02:38:53Z

Hey @Debrice that was "unfixed" when I added closeOnTriggerClick. Here's the diff that will fix it if you want to just add it on your branch:

--- a/src/addons/Portal/Portal.js
+++ b/src/addons/Portal/Portal.js
@@ -205,14 +205,14 @@ class Portal extends Component {
       e.stopPropagation()
       this.close(e)
     } else if (!open && openOnTriggerClick) {
-      // Prevents closeOnDocumentClick from closing the portal when
-      // openOnTriggerFocus is set. Focus shifts on mousedown so the portal opens
-      // before the click finishes so it may actually wind up on the document.
-      e.nativeEvent.stopImmediatePropagation()
-
       e.stopPropagation()
       this.open(e)
     }
+
+    // Prevents closeOnDocumentClick from closing the portal when
+    // openOnTriggerFocus is set. Focus shifts on mousedown so the portal opens
+    // before the click finishes so it may actually wind up on the document.
+    e.nativeEvent.stopImmediatePropagation()
   }

   handleTriggerFocus = (e) => {

Also, can you squash your commits back down again? The reason your rebase was difficult last time was because you had merged a few times before and git had to try to parse through all the merged commits with duplicate code.

Once you squash again, any time you want to pull in the latest changes from master if you use rebase you shouldn't have any conflicts and your work will be back on top without any merge commits.

typehorror · 2016-10-05T02:44:30Z

Great, thank you very much @jcarbo, this fixes the issue indeed! I will do the rebase very soon, once I'm done wrapping up the unit test.

typehorror · 2016-10-05T03:46:35Z

@levithomason I think we're ready for the final review.

jeffcarbs · 2016-10-05T04:00:09Z

src/modules/Popup/Popup.js

+
+    const style = _.assign({}, this.state.style, this.props.style)
+
+    const classes = cx(


Update usage of cx to follow the standard pattern of this lib (see https://github.com/TechnologyAdvice/stardust/blob/master/src/collections/Grid/Grid.js for an example). There are helpers to set the appropriate class based on the prop.

For example, wide here can use the useKeyOrValueAndKey (see similar prop usage: https://github.com/TechnologyAdvice/stardust/blob/master/src/collections/Grid/Grid.js#L34-L35)

jeffcarbs · 2016-10-05T04:06:02Z

There's still quite a few lines missing test coverage. We keep this package at 100% test coverage so we won't be able to merge until all lines are covered. You can see the coverage by clicking details here:

You can also install the codecov.io browser extension to see the coverage right in github:

typehorror · 2016-10-05T04:14:14Z

@jcarbo sounds good, I'll work on getting 100% coverage. Thanks for the heads up

typehorror · 2016-10-05T07:50:13Z

@jcarbo I'm almost done. I'm missing some positioning/sizing tests for which I haven't yet find a solution.

levithomason · 2016-10-05T17:40:22Z

Coverage Report

You can also get a killer coverage report by opening:

file:///<path_to_project>/coverage/lcov-report/index.html

Where <path_to_project> is your absolute file path to the project root.

Investigating Coverage

You can sort, drill down to directories/files, and see specific lines:

TYPO: I meant, when positions does NOT include 'right' :)

typehorror · 2016-10-05T18:38:41Z

Thanks @levithomason, I found the coverage yesterday evening after @jcarbo comment but I really appreciate how all of you go above and beyond to guide and help.

I know it's a bit late but, is there some guide I should read ? Also, which element would incarnate the project code style (including test and example) ?

For example, many example are using Components while being stateless which turns out to be something we don't want. If we could illustrate guidance through an exemplary implementation of a SUI element I believe a new contributor (like myself) could save you a lot of time.

Finally, testing the positioning logic turns out to be quite a puzzle, I really want to to cover it fully prior to release but it might take a few more days.

levithomason · 2016-10-05T19:20:26Z

I know it's a bit late but, is there some guide I should read ? Also, which element would incarnate the project code style (including test and example) ?

Not sure if you've read the CONTRIBUTING.md yet. It is a walkthrough from cloning the repo, to documenting and testing your component. So, it is meant to be read like a book, from top to bottom. It is fairly comprehensive, but could always be better. LMK if you feel something is missing from there.

typehorror · 2016-10-05T19:26:30Z

@levithomason did the CONTRIBUTING.md get deleted?

levithomason · 2016-10-05T19:28:45Z

Whoops, no it moved to .github. I had an old link, updated, try it now 👍

codecov-io · 2016-10-06T03:41:22Z

Current coverage is 100% (diff: 100%)

Merging #570 into master will not change coverage

@@           master   #570   diff @@
====================================
  Files         119    122     +3   
  Lines        1890   2000   +110   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         1890   2000   +110   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update 586bdd4...e68874d

typehorror · 2016-10-06T03:45:52Z

@jcarbo and @levithomason, the branch finally got a 100% code coverage 🎉

I believe I could test the library more but I need more research on phantomJS (event.target.getBoundingClientRect returns 0 for all coordinates).

typehorror · 2016-10-06T23:43:25Z

@levithomason I think we are good for the final review (unless there is something else I didn't see) 😄

levithomason · 2016-10-08T04:22:26Z

Cool, this looks like a great first shipment for this component. Thanks for hanging in there and pioneering this stuff!

levithomason · 2016-10-08T04:37:26Z

Released in semantic-ui-react@0.54.5

typehorror · 2016-10-08T06:01:09Z

Thank you guys for your patience and especially @jcarbo for his great work on porting and upgrading the Portal lib! I had a blast 👍

typehorror mentioned this pull request Sep 30, 2016

<Popup /> Component #193

Closed

layershifter reviewed Sep 30, 2016

View reviewed changes

src/modules/Popup/Popup.js

)

this.setState({ popup })

}

Copy link

Member

layershifter Sep 30, 2016

Choose a reason for hiding this comment

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

Popup will be rerendered on each show.

levithomason requested changes Oct 1, 2016

View reviewed changes

typehorror changed the base branch from master to feature/modal October 1, 2016 03:16

typehorror changed the base branch from feature/modal to master October 1, 2016 03:17

jeffcarbs reviewed Oct 1, 2016

View reviewed changes

levithomason requested changes Oct 1, 2016

View reviewed changes

levithomason changed the title ~~wip(popup): docs and proof of concept~~ feat(Popup): add component Oct 1, 2016

levithomason added the enhancement label Oct 1, 2016

levithomason reviewed Oct 1, 2016

View reviewed changes

levithomason mentioned this pull request Oct 1, 2016

RFC: remove production propTypes, allow small custom builds #524

Closed

47 tasks

levithomason requested changes Oct 4, 2016

View reviewed changes

levithomason added the needs author feedback label Oct 4, 2016

layershifter reviewed Oct 4, 2016

View reviewed changes

jeffcarbs reviewed Oct 5, 2016

View reviewed changes

jeffcarbs added ready for review and removed needs author feedback labels Oct 7, 2016

feat(Popup): Add component

9a5df1c

levithomason merged commit b1c5be9 into Semantic-Org:master Oct 8, 2016

pdfowler mentioned this pull request Apr 6, 2018

Popup's do not work with modal's when dimmers are enabled. #1803

Open


		const style = _.assign({}, this.state.style, this.props.style)

		const classes = cx(

feat(Popup): add component #570

feat(Popup): add component #570

Conversation

typehorror commented Sep 30, 2016 • edited Loading

codecov-io commented Sep 30, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

typehorror commented Sep 30, 2016 • 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 Sep 30, 2016 • edited Loading

typehorror commented Sep 30, 2016 • edited Loading

jeffcarbs commented Sep 30, 2016 • edited Loading

typehorror commented Sep 30, 2016 • edited Loading

jeffcarbs commented Sep 30, 2016

typehorror commented Sep 30, 2016

levithomason 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

typehorror Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Sep 30, 2016 • edited Loading

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

typehorror Oct 1, 2016 • edited Loading

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

levithomason commented Oct 1, 2016

typehorror commented Oct 1, 2016

Choose a reason for hiding this comment

typehorror Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Oct 1, 2016

levithomason 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

levithomason Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Oct 1, 2016 • edited Loading

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

levithomason Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

jeffcarbs commented Oct 2, 2016

typehorror commented Oct 2, 2016 • edited Loading

typehorror commented Oct 2, 2016

jeffcarbs commented Oct 2, 2016

typehorror commented Oct 2, 2016 • edited Loading

levithomason commented Oct 3, 2016 • edited Loading

jeffcarbs commented Oct 3, 2016 • edited Loading

levithomason commented Oct 4, 2016

typehorror commented Oct 4, 2016

levithomason left a comment

Choose a reason for hiding this comment

levithomason Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

typehorror commented Sep 30, 2016 •

edited

Loading

codecov-io commented Sep 30, 2016 •

edited

Loading

typehorror commented Sep 30, 2016 •

edited

Loading

layershifter commented Sep 30, 2016 •

edited

Loading

typehorror commented Sep 30, 2016 •

edited

Loading

jeffcarbs commented Sep 30, 2016 •

edited

Loading

typehorror commented Sep 30, 2016 •

edited

Loading

typehorror Oct 1, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading

typehorror Oct 1, 2016 •

edited

Loading

typehorror Oct 1, 2016 •

edited

Loading

levithomason Oct 1, 2016 •

edited

Loading

levithomason Oct 1, 2016 •

edited

Loading

levithomason Oct 1, 2016 •

edited

Loading

levithomason Oct 1, 2016 •

edited

Loading

typehorror commented Oct 2, 2016 •

edited

Loading

typehorror commented Oct 2, 2016 •

edited

Loading

levithomason commented Oct 3, 2016 •

edited

Loading

jeffcarbs commented Oct 3, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

`position='top center'`

`position='left center'`

`position='bottom center'`

layershifter Oct 4, 2016 •

edited

Loading

typehorror commented Oct 5, 2016 •

edited

Loading

jeffcarbs Oct 5, 2016 •

edited

Loading