search

GriddleGriddle / Griddle

Simple Grid Component written in React
http://griddlegriddle.github.io/Griddle/
MIT License
2.5k stars 378 forks source link

Extract Initializer #757

Closed dahlbyk closed 7 years ago

dahlbyk commented 7 years ago

Griddle major version

1.x

Changes proposed in this pull request

Pulls the default+plugin+props composition out of the Griddle constructor into a testable initializer.

This is really just prep to work on supporting function plugins for #733.

Why these changes are made

Tests are good, especially for this sort of composition (which seems likely to get more complex).

Are there tests?

Lots!

dahlbyk commented 7 years ago

I am seeing some issues on the TS front in storybook that I'm having a hard time tracking down. I might need to lean on @ryanlanciaux to investigate that bit. Are you seeing anything?

I don't recall any issues from a month ago when I was working on this, but I can take another look.

I take it you approve of this direction? If so, I'll rebase on master to account for evolution since then.

dahlbyk commented 7 years ago

Inspired by #760, I've added commits to pull the current defaults into a CorePlugin of sorts that can be replaced (or omitted) via <Griddle core={...} />. Not sure if core is the right name for it, but it does seem a useful extra seem to adjust Griddle default behavior.

I did not bring in @shorja's core support for accepting a function from props, so parsing renderProperties from children (RowDefinition/ColumnDefinition) is the only behavior that's really not extensible. I'm not thrilled with core as function-of-props because for #733 I have been thinking of plugins as more of a reducer. But I'm not sure what a better pattern for handling renderProperties would be.

shorja commented 7 years ago

@dahlbyk Yeah now that I look at the core plugin taking in props, that's probably not a great way to handle stuff, especially since the pattern seems to be passing in init config to a plugin is done outside griddle -> . So really anything that should go into core really should be passed in like that, props should have nothing to do with it.

Is the idea of plugins as reducers to basically chain the output of built plugins into the next plugin to build and then each one can do what it wants to wrap components? I've also been thinking about refactoring the component composition logic, included in my mega PR was a change to allow enhancers to 'stack' but I was planning on revisiting it. I drew up a diagram that explains the behaviour I was thinking about.

componentcomposition00

dahlbyk commented 7 years ago

Is the idea of plugins as reducers to basically chain the output of built plugins into the next plugin to build and then each one can do what it wants to wrap components? I've also been thinking about refactoring the component composition logic, included in my mega PR was a change to allow enhancers to 'stack' but I was planning on revisiting it.

Yeah, given a series of function plugins, you can think of initializer as essentially plugins.reduce((config, plugin) => plugin(config), core). My first commit in this direction was https://github.com/GriddleGriddle/Griddle/commit/e3f9481af3693160e745ff0f312f998722cbca34, but I wanted to land this initial refactoring first.

I like the idea of being able to apply multiple enhancers, as your diagram explains, but explaining "enhancers cleared by a new component" rules might be less intuitive than making (function) plugins decide how old/new enhancers should compose (c.f. https://github.com/GriddleGriddle/Griddle/issues/733#issuecomment-330975239). I'd suggest the plugin composition conversation continue over there.

ryanlanciaux commented 7 years ago

I like this direction and definitely wonder if pushing the enhancer registration logic to the plugins themselves would make the logic easier to understand for anyone using the library.

dahlbyk commented 7 years ago

I am seeing some issues on the TS front in storybook that I'm having a hard time tracking down. I might need to lean on @ryanlanciaux to investigate that bit. Are you seeing anything?

I don't recall any issues from a month ago when I was working on this, but I can take another look.

@joellanciaux are you still seeing this? Thoughts on the pageProperties/sortProperties change?

ryanlanciaux commented 7 years ago

I'm in favor of moving this one forward. If there are no storybook + typescript issues (I'm not seeing any atm) I will merge this. Sound good? :D