Big Refactoring and adding possibility of className prop #57
Conversation
lowly2005
changed the title
|
Wow, seems to be a very big one, I will need some time to read through it |
@lowly2005, I don't see this enhancement, only changes to examples. |
Redmega
changed the title
| style: {style: {}, arrowStyle: {}} | ||
| }; | ||
|
|
||
| const propTypes = { |
There was a problem hiding this comment.
I liked the static props and defaultProps. What was the thought process behind doing it this way?
There was a problem hiding this comment.
It keeps the class clean, declaring const propTypes and assign them later to the static properties. + it will be easy to reuse data structures across components.
defaultProps are const because they will not change through the program.
| } | ||
| Card.defaultProps = defaultProps; | ||
| Card.propTypes = propTypes; | ||
| export default Card; |
| box-shadow: 0 0 8px rgba(0,0,0,.3); | ||
| border-radius: 3px; | ||
| z-index: 50; | ||
| } |
There was a problem hiding this comment.
I see that you have put all the styles into one css file. Maybe we can start doing http://getbem.com/introduction/ while we are at it.
For example, there is one big block ReactPortalTooltip and then ReactPortalTooltip__card
There was a problem hiding this comment.
@tanqhnguyen yes it's a good suggestion.
| import ListItems from './ListItems' | ||
|
|
||
| class Groups extends Component { | ||
| constructor(props) { |
There was a problem hiding this comment.
This constructor doesn't do anything, maybe just remove it?
| this.showTooltip = this.showTooltip.bind(this); | ||
| this.hideTooltip = this.hideTooltip.bind(this); | ||
| this.state = { | ||
| isTooltipActive: false |
There was a problem hiding this comment.
indentation is a bit weird here
|
|
||
| var portalNodes = {}; | ||
|
|
||
| class CardWrapper extends Component { |
There was a problem hiding this comment.
Not sure how important is it but I think we should name this one Tooltip because it basically replaces the previous Tooltip class
There was a problem hiding this comment.
I mean in https://github.com/romainberger/react-portal-tooltip/blob/master/src/index.js we have a class called ToolTip which does exactly what your CardWrapper does. Therefore I think we should rename CardWrapper to ToolTip
| } | ||
| constructor(props) { | ||
| super(props); | ||
| this.showTooltip = this.showTooltip.bind(this); |
There was a problem hiding this comment.
the convention is to not have ;
| box-shadow: 0 0 8px rgba(0,0,0,.3); | ||
| border-radius: 3px; | ||
| z-index: 50; | ||
| } |
There was a problem hiding this comment.
I see that you have put all the styles into one css file. Maybe we can start doing http://getbem.com/introduction/ while we are at it.
For example, there is one big block ReactPortalTooltip and then ReactPortalTooltip__card
|
Also, the convention is to not have |
| active: PropTypes.bool, | ||
| parentEl: PropTypes.object, |
There was a problem hiding this comment.
Since parentEl is required in the outer component, maybe we should mark it as required here as well?
|
I'm not sure about moving the style into a css file. In theory I like it, but it means it forces people using the module to have a css loader and to include the node_modules, and that bothers me. I'd rather keep it inline since there's not a lot of style anyway. |
|
Closing as stale. Feel free to update and reopen. |
Overview:
PR contains component enhancement: possibility to add className for custom css.
It contains also an update of npm modules like react-router , babel, webpack etc. and a refactoring of the example and the main src project
@romainberger If you have any suggestions feel free to poke me
How to use: The readme is updated.
Resolves #36