-
Notifications
You must be signed in to change notification settings - Fork 378
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
Hover plugin #639
base: master
Are you sure you want to change the base?
Hover plugin #639
Conversation
@dahlbyk Any news on merging this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelDeBoey I'll defer to @ryanlanciaux @joellanciaux on the decision to include another plugin, and if so what that plugin scope should be.
In the meantime, here are some thoughts on the code.
focusOut: rowMouseLeave | ||
}, dispatch) | ||
), | ||
components.RowContainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than compose
this with the default RowContainer
, it's probably more correct for this to be a RowEnhancer
(like in this example story).
}, | ||
(dispatch, {griddleKey}) => bindActionCreators({ | ||
focusIn: rowMouseEnter.bind(this, griddleKey), | ||
focusOut: rowMouseLeave |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of #679, if these were onMouseEnter
/onMouseLeave
they'd be passed through correctly in Row
.
connect( | ||
state => { | ||
return { | ||
hoveredRowKey: state.get('hoveredRowKey', null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely convinced it should be the plugin's responsibility to inject hoveredRowKey
into Cell
(or anywhere). It seems to be an implementation detail of this demo. I'm more inclined to avoid customizing Row
altogether, letting ButtonsColumn
be connected instead:
const ButtonsColumn = connect(
state => ({ hoveredRowKey: state.get('hoveredRowKey', null) })
)(
({griddleKey,hoveredRowKey}) =>
(griddleKey === hoveredRowKey) && <button>Edit</button> : null
);
@yoursdearboy Any news on addressing @dahlbyk's comments? 🙂 |
@MichaelDeBoey honestly, I would just copy the relevant bits into your own plugin. There's not all that much to wiring up |
@dahlbyk Yeah I had a few problems doing so, so was hoping the plugin could help 😕 Will try and make some issues together with a codesandbox, so you could maybe point me in the right direction if you want 🙂 |
Glad to help as time allows |
Griddle major version
1.3.0
Changes proposed in this pull request
Added a plugin that provide each
Cell
withhoveredRowKey
property which is equal togriddleKey
of hovered row.Why these changes are made
Don't know if this repo is a proper place to contribute plugins (at least I see plugins directory).
We use it to show toolbar for records in rows (see storybook demo).
Are there tests?
Storybook story only.