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

[ModularTable] Add prop for plugin passthrough #440

Open
hatched opened this issue Apr 14, 2021 · 5 comments
Open

[ModularTable] Add prop for plugin passthrough #440

hatched opened this issue Apr 14, 2021 · 5 comments
Labels
P3 Needs decision Needs team agreement in terms of triaging.

Comments

@hatched
Copy link
Contributor

hatched commented Apr 14, 2021

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

hatched commented Apr 15, 2021

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

bartaz commented Apr 15, 2021

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

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

hatched commented Apr 16, 2021

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

bartaz commented Apr 16, 2021

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.

@bartaz bartaz added the P3 Needs decision Needs team agreement in terms of triaging. label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Needs decision Needs team agreement in terms of triaging.
Projects
None yet
Development

No branches or pull requests

3 participants