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

M major refactor #760

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Conversation

shorja
Copy link

@shorja shorja commented Oct 19, 2017

Griddle major version

1.10.0

Changes proposed in this pull request

A lot, this is sort of an 'omnibus' PR that includes all of the Griddle refactor stuff I've been working on. Unfortunately its gotten to the point where this stuff makes most sense all being together. This is not meant to be merged, but to be a place to centralize the conversation about the changes I'm proposing and also makes it easier to run them all at once. This branch is currently very active so expect frequent updates. Also, if this is not a good way of approaching these changes please let me know.

  • Griddle now only uses plugins to build the table. I have moved all of the table code in src/ into src/plugins/core, I have been referring to this stuff as the 'core' plugin. By default this 'core' plugin is used as the baseline to start building Griddle but now you can pass in a prop the is a different baseline plugin. This is a big change to the Griddle component itself, the basic idea though is that there shouldn't be any dependencies on any actual table code when building the plugins, we should just get components, reducers, selectors, actions, and store state from each plugin and layer those on top of each other. My intention is to make Griddle at its core no longer a table library, but a component building framework.

  • I'm trying to move everything any individual component might depend on to the context. This improves the already great behaviour overriding features Griddle already has since it creates a single point of truth for the props that are mapped into the components.

  • Selectors are probably the single biggest change I've been working on, and I still have more improvements I want to make here. As has been discussed in this PR selectors are now individually overridable via the use of a special griddleCreateSelector function and composeSelectors utility function. I have added some improvements on that pull request here: griddleCreateSelector now supports 'mixed mode' selector creation where you can use a combination of literal selector function and string selector dependencies. It now also will override every single instance of a selector being defined so imported selectors now automagically get the proper overridden behaviour. Selectors now also have a factory prop on them so you can get individual instances of each selector. The factory also accepts an object that allows you to redefine the dependency selectors for every run of the factory. I have improvements coming soon to allow very easy factory-ified creating of selectors for components that are not rendered just once, rows and cells being the most common use cases.

  • Actions have also been moved to the context. This object is currently very simple, just all of the actions defined in the plugins. I would like to take a closer look at this and see how overriding should be handled.

  • Multiple Enhancers can now be applied to a given component.

Why these changes are made

Generally to support improving the plugin and overriding behaviour of Griddle overall. The table we are building has a ton of features which can be mixed and matched and so making the infrastructure really really solid is our primary motivation.

Are there tests?

Testing is ongoing, I have been adding tests for the selector composition utility, and I have updated existing tests to function properly with this refactor.

Short, James and others added 25 commits September 26, 2017 01:16
…a composable selector generator function to be used to create composable selectors.
* master:
  Use Loading component when data={null|undefined}
  Handle data={null|undefined}
  Wire up Redux DevTools
* m-actions-on-context:
  Add actions to the context. Use the simple later plugins override method for now.
…d imports of selectors and moved import statements of any Griddle code to be separated from module imports
* refactor-743:
  Convert existing selectors to dynamic dependency resolution
  Composable selectors

Also refactors on top of refactor-743
…rking fine with this code. Going to pull a lot of the 'core' specific stuff into that plugin so that the initialization phase has no dependencies on its specific structure.
…as been moved out. There are still some lingering bits to do with the redux state I would like to fix but they need selector refactors.
* m-baseline-plugin:
  Most of the stuff associated with the 'core' plugin in src/index.js has been moved out. There are still some lingering bits to do with the redux state I would like to fix but they need selector refactors.
  First pass at moving 'core' code into a plugin. Stories seem to be working fine with this code. Going to pull a lot of the 'core' specific stuff into that plugin so that the initialization phase has no dependencies on its specific structure.

# Conflicts:
#	src/index.js
* m-composable-selectors-refactor:
  Complete example story
  add dataLoadingSelector to composedSelectors
  Refactor and add story
  Convert existing selectors to dynamic dependency resolution
  Composable selectors
  Refactored the selector composing function into a utils class, added a composable selector generator function to be used to create composable selectors.
  Composable selectors

# Conflicts:
#	src/index.js
#	src/plugins/core/selectors/dataSelectors.js
* m-selectors-actions-on-context:
  Removed hard references to actions in the core code, deleted commented imports of selectors and moved import statements of any Griddle code to be separated from module imports
  Add actions to the context. Use the simple later plugins override method for now.
  Small changes to TableContainer
  Move selectors on context change to its own branch

# Conflicts:
#	src/index.js
#	src/plugins/core/components/CellContainer.js
#	src/plugins/core/components/FilterContainer.js
#	src/plugins/core/components/LayoutContainer.js
#	src/plugins/core/components/LoadingContainer.js
#	src/plugins/core/components/NextButtonContainer.js
#	src/plugins/core/components/NextButtonEnhancer.js
#	src/plugins/core/components/NoResultsContainer.js
#	src/plugins/core/components/PageDropdownContainer.js
#	src/plugins/core/components/PaginationContainer.js
#	src/plugins/core/components/PreviousButtonContainer.js
#	src/plugins/core/components/PreviousButtonEnhancer.js
#	src/plugins/core/components/RowContainer.js
#	src/plugins/core/components/SettingsContainer.js
#	src/plugins/core/components/SettingsToggleContainer.js
#	src/plugins/core/components/SettingsWrapperContainer.js
#	src/plugins/core/components/TableBodyContainer.js
#	src/plugins/core/components/TableContainer.js
#	src/plugins/core/components/TableHeadingCellContainer.js
#	src/plugins/core/components/TableHeadingCellEnhancer.js
#	src/plugins/core/components/TableHeadingContainer.js
* m-composable-enhancers:
  Roll back some changes
  Compose enhancers instead of overriding them
…s 'beginning' oldState instead of undefined, updated the core GRIDDLE_UPDATE_STATE reducer to not attempt a change on data if it is undefined. This may warrant some further looking at. Also updated the story for overridable selectors so that it should be working fine now.
…rs and directory structure. Selectors now have a factory prop that can build copies of this selector.
@dahlbyk dahlbyk mentioned this pull request Oct 20, 2017
@dahlbyk
Copy link
Contributor

dahlbyk commented Oct 20, 2017

Related: #757 (comment)

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.

None yet

2 participants