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

Hover plugin #639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yoursdearboy
Copy link

Griddle major version

1.3.0

Changes proposed in this pull request

Added a plugin that provide each Cell with hoveredRowKey property which is equal to griddleKey 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.

@MichaelDeBoey
Copy link

@dahlbyk Any news on merging this one?

Copy link
Contributor

@dahlbyk dahlbyk left a 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
Copy link
Contributor

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
Copy link
Contributor

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)
Copy link
Contributor

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
  );

@MichaelDeBoey
Copy link

@yoursdearboy Any news on addressing @dahlbyk's comments? 🙂

@dahlbyk
Copy link
Contributor

dahlbyk commented Dec 20, 2017

@MichaelDeBoey honestly, I would just copy the relevant bits into your own plugin. There's not all that much to wiring up Row events.

@MichaelDeBoey
Copy link

MichaelDeBoey commented Dec 20, 2017

@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 🙂

@dahlbyk
Copy link
Contributor

dahlbyk commented Dec 23, 2017

Glad to help as time allows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants