Cleanup Datatable / withComponents pattern

[eric-burel]

Done so far:

• split the visual component of DataTable. It makes it quite verbose, with many small components, but it will make maintaining libs such as vulcan:material-ui far easier (@ErikDakoda).
  Basically this way the only components that needs to be replaced to change the frontend lib now contain zero logic. So we can avoid discrepancies between UI libs every time we update Vulcan.

• expose a withComponents HOC that fetch the global Components object, merge it with a local components prop if its present (or formComponent until we cleanup the Forms again), and pass a Components prop.
  Again this makes the core component implementation verbose (see Datatable), but cleaner. Also, the logic of the datatable could be reused with very different designs.

• expose mergeWithComponents too (less useful though)

This is quite experimental, I'd like to get some feedback on this. I did not observe regressions with Vulcan material-ui for the moment (I added components but did not change existing one that much).

Note that I don't really expect end users to use such features, as they concerns only components that tends to be very reusable (SmartForm, Datatable) and thus should tolerate advanced customization.

[eric-burel]

To provide an example of local replacement this code could replace the table by a grid of items for example:
https://github.com/lbke/awesome-vulcan/blob/experiment-datatable/packages/core/lib/components/ResultsGrid.jsx

[SachaG]

I didn't really follow everything but overall it seems like a good idea. I agree making Datatable a bit more verbose is fine if it makes it easier to customize it. Another thing to remember is that we can sometimes use class components to extract a component's logic into a method, and then only override the render() method when we replace the component.

[eric-burel]

Can you elaborate what you mean by overriding the render method? By creating an HOC or relying on a render props for example? In the Datatable case there is not much logic, except maybe in the main component that we could refactor a little, but indeed I'll keep this in mind for the SmartForm for example.

[SachaG]

No I just meant that in, for example, the Datatable component, this:

render() {

  const DatatableWithMulti = withMulti(options)(Components.DatatableContents);
  
  // ...

  return (...)
}

could be refactored as this:

getWrappedComponent() {
  return withMulti(options)(Components.DatatableContents);
}

render() {

  const DatatableWithMulti = this.getWrappedComponent();
  
  // ...

  return (...)
}

This way if you want to replace Datatable yourself you can do class MyDatatable extends Components.Datatable and only worry about changing the render, no about rewriting the logic used to wrap the component.

Basically anything we have more than JSX code in the render, it's an indication that maybe that code should be refactored out into a method that can be inherited when extending the component.

[eric-burel]

Okay I get what you meant. I've just fixed the last commits and tested the code, it works as expected both with and without Material UI, I think we can merge. I'll come back at the DataTable later when I will be on a new project

[Apollinaire]

Let me add my view on this. I think using class components in which the render method only serves for displaying, and all the logic is inside prototype methods is a valid option but it does not scale well with themes and amount of components.
IMO, the best approach is to split logic and render package-wise, not just inside the component. Because I don't want to have stuff from the default theme in my material-ui based app, or in any other theme. When we have 1000 components in the core, the amount of useless render methods will be huge, thus bloating the app.

The approach described by Sacha is a good option for now, it makes things work really well and it keeps stuff at one place so it's easy to maintain for now, while we don't define that many components. And we should keep using this to make logic replaceable too.

In the end, I want to be able to switch from bootstrap, to bulma, to material-ui, or ant-design with just two-lines:

meteor remove vulcan-ui-bootstrap
meteor add vulcan-ui-antd

and ideally it would work out of the box (modulo the custom css / colors / settings / whatever the ui lib requires).
This is really not far from working. I was amazed last weekend how the AccountsLoginForm worked so well on Bulma with only setting FormControl and Button components. This is what I want to achive with every component in the core package of Vulcan. In the end, it will reduce our bundle size by removing redundant code and useless dependency.

So IMO, the initial approach of Eric is good. Even better if we add the method splitting explained by Sacha. It's hard work at the core of Vulcan, but if we do the hardwork at the core of the framework, then no one else has to do it. And that's how the framework will shine and gain momentum.

PS: on a more meta note, I want to say that I love working with Vulcan (hence with all its community), and there is so much I want to do to improve it.

[SachaG]

@eric-burel sounds good, feel free to merge this when you have time.

@Apollinaire thanks for the meta note! I also love this community :)

[eric-burel]

I'll merge for now, I'll stay reachable next week monday or tuesday if this provokes any bug on the Datatable.