canonical / react-components

A set of components based on Vanilla Framework
https://canonical.github.io/react-components
101 stars 55 forks source link

[ModularTable] Add prop for plugin passthrough #440

Open hatched opened 3 years ago

hatched commented 3 years ago

React Table useTable takes n arguments after the first for plugins.

 const instance = useTable(
   {
     data: [...],
     columns: [...],
   },
   useGroupBy,
   useFilters,
   useSortBy,
   useExpanded,
   usePagination
 )

In order to implement the grouping in the Juju Dashboards Action Logs UI it looks like using useExpanded with the default be expanded might be the best way to implement this functionality. To this end I'd like to expand ModularTable to take a plugins prop to be expanded out when calling useTable.

III  Action logs - Default view

hatched commented 3 years ago

To make this truly functional we would also have to take an options prop that would be expanded out into the useTable options argument so that configuration options could be passed in to the defined/supplied hooks. Something like this (untested, psudo code)

const instance = useTable.call(this, [{
  data,
  columns,
  ...configOptions
}, ...extensionHooks]);

I had first explored the idea of having boolean values to enable/disable the hooks that we want to support but I figured keeping the options and plugin hooks open allowed for a lot more flexibility.

bartaz commented 3 years ago

Thanks Jeff, it's an interesting idea. As far as I remember I did initially intend to expose the internals of ModularTable more.

One reasons I haven't done that is that it seems to allow and encourage building custom solutions in products rather than implementing the consistent behaviour as intended by modular table design.

I'm afraid that by allowing to pass hooks into ModularTable, product teams will rather use that when needed (for example for grouping) rather than contribute to 'official' grouping in the ModularTable component.

The other thing is - what to do with custom props that may override the built-ins? Currently we don't use any plugins internally in ModularTable, but once we start it's a valid risk. If we allow to override the plugins and options it may lead to unexpected behaviours or errors.

I'm keen on the idea of flexibility, I just don't know how to make it work well with consistent built-in functionality.

anthonydillon commented 3 years ago

I also share Bartek's concern regarding custom implementations. In practise, we have less than 10 tables build with React across the team. So we should be able to build each one from built in functionally.

@hatched which part of the design attached requires an extension?

hatched commented 3 years ago

After discussing this with @huwshimi and @Caleb-Ellis we've come to a different proposal that I think works quite well. Instead of enabling or disabling plugins and supplying configuration values as separate props we will use a more data driven approach and enable or disable features based on the dataset passed in.

In the original case, if you pass a dataset with subRows then we will generate a table with the useExpanded plugin enabled. So with this we can control which features are enabled by default and this feature will "just work". It also allows us to robustly test this functionality and provide documentation for it. This is in contrast to a fully open config and plugin object that is up to the end user to test and determine how to configure the table.

For the second case in this following image we need to be able to expand or collapse the rows depending on an out-of-table UI element. III  App - Unit - Show subordinates

To accomplish this we can add a new prop that's simply collapseSubRows: boolean to gain external control. If we want to enable selective control over which elements are opened and closed from outside of the table this same prop can instead take an object of Id's. This will work better than simply controlling the data as we will be able to take advantage of the built-in collapse functionality and provide smooth transitions (animations) between the two states.

As for additional plugins. I expect that we'll need to continuously add new features as time goes on but by going with a data driven approach and then adding simple props for the more advanced functionalities should allow us to keep the API surface area to a minimum. 🤞

bartaz commented 3 years ago

Instead of enabling or disabling plugins and supplying configuration values as separate props we will use a more data driven approach and enable or disable features based on the dataset passed in.

@hatch That sounds good. Thanks for the update.

Having quickly a look at useExpanded example here it seems that technically useExpanded plugin can be always there in ModularTable, and basically depending on the columns/data passed to the table it will be used or not. I'm not sure exactly if that will work, or what are the drawbacks of having a plugin "enabled" when it's not used, but seems to be an option to consider and try.