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

Allow to pass custom table components #1167

Merged
merged 4 commits into from
Jun 1, 2020
Merged

Allow to pass custom table components #1167

merged 4 commits into from
Jun 1, 2020

Conversation

vbartusevicius
Copy link

@vbartusevicius vbartusevicius commented Jan 23, 2020

Currently there is no way to change built-in table components. This PR enables some customization.
This is a bit different that customizing with custom Material theme as this allows to react to given props.

In example: you can change the color of Chip in TableFilterList depending on current value. Useful for filtering by some status-like fields - you will have one Chip indicating status with visual feedback in filter list instead of duplicating this feedback throughout entire table.

image

@coveralls
Copy link

coveralls commented Jan 23, 2020

Coverage Status

Coverage increased (+1.06%) to 77.497% when pulling b06a025 on vbartusevicius:customizable-components into 4f83609 on gregnb:master.

@vbartusevicius vbartusevicius requested a review from gregnb January 23, 2020 16:27
@vbartusevicius
Copy link
Author

@gregnb, @gabrielliwerant - could you take alook at this?

@gabrielliwerant
Copy link
Collaborator

@vbartusevicius There is a rather long backlog of PRs and issues, and they should be taken somewhat in order, unless it's a quick bug fix or something. This doesn't qualify, and so I'm afraid it may be a while. Additionally, I'm not too sure about this approach. The way customization tends to be handled now is through custom renderers. There's no precedent in the library for customization that works in the way you propose, so I'm not too sure about adding it.

@vbartusevicius
Copy link
Author

@gabrielliwerant, yes there is a long backlog, but I don't see too much of activity there. I think this PR could help with cases like #351 In addition to that I intentionally introduced separate prop components as I think it brings more separation between purposes of props - now there is a bit of a mess: handlers, customComponents, options, configurations, etc. in one options prop.

If you think that it would be more reasonable to move components to individual options.custom* - I'm fine with that.

@patorjk patorjk changed the base branch from master to v3 June 1, 2020 17:01
@patorjk patorjk merged commit 4f5d32f into gregnb:v3 Jun 1, 2020
@patorjk
Copy link
Collaborator

patorjk commented Jun 1, 2020

This is an exceptionally cool idea. Currently much of what's described here can be done with the table's various customRender functions, however, this type of API (to me at least) is much easier to understand. This approach would also solve the Tooltip component problem described in #1217 (though this PR doesn't have Tooltip available as an override so I may look into that). It also lends itself well to the new plug-in selection, and would allow alternative forms of various components for common use cases.

The downsides I see are that if any prop name to one of these overridable components changes, the custom component will break. But these names have been pretty stable, and maybe saving such a change for a major release would be the way to go if such a situation arose. There could also be issues if any data formats changed, but customRender functions have this same issue. I may add something in the documentation about recommending the use of ~versions for apps that do lots of custom internal overrides.

Anyway, I've merged this into the upcoming v3 branch. Sorry for the delay and thank you for putting this together!

@patorjk patorjk mentioned this pull request Jun 1, 2020
@vbartusevicius
Copy link
Author

@patorjk - thanks for appreciation!

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.

4 participants