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

feat: add container to arguments passed to dataGetter #120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JRGranell
Copy link

@JRGranell JRGranell commented Nov 22, 2019

This PR adds the container property to the argument list in the dataGetter call in the same as the cellRenderer call.

I would like to be able to access custom props that are passed to the container in the data getter function. Currently this is supported in the cellRenderer however we don't need the power of the that function (cell rendering for us is taken care of elsewhere).

This PR aligns the `dataGetter` and `cellRenderer` function calls by ensuring the container property is passed.

I would like to be able to access custom props that are passed to the container in the data getter function. Currently this is supported in the `cellRenderer` however we don't need the power of the that function (cell renderering for us is taken care of elsewhere).
@JRGranell JRGranell changed the title Add container to arguments passed to dataGetter feat: add container to arguments passed to dataGetter Nov 22, 2019
@nihgwu
Copy link
Contributor

nihgwu commented Nov 22, 2019

Thanks for your contribution, the container is initially designed to provide container for Portal/Overlay, not accessing the internal state and props, and the dataGetter is only for simple string display, so no container is needed, that's why container is not provided in dataGetter. For user land, the consumer should always know the table's props as he is passing in the props. On the other hand, dataGetter is used to get cellData and be used in TableCell, so you can simply use another property to column like formatter and passing what ever you have.

I'm not discourage you and suggest you to drop your suggestion, indeed in the next major version I planed to add container to all callbacks like dataGetter and rename it to table to make it more general, and your use case is the best example. But I decided to suspend new features for V1 unless it's really important and there is no workaround, while there is for your case.

@nihgwu
Copy link
Contributor

nihgwu commented Nov 22, 2019

Let me know your thoughts, and if you still think it's worth to be supported, you should merge to v1 branch so I can publish patches soon

@JRGranell
Copy link
Author

Thanks for the quick response, you make a good point. It was in response to this thread #27 (comment) as an alternative implementation. Let me have another look and see if there is another approach that could be taken.

@nihgwu
Copy link
Contributor

nihgwu commented Nov 22, 2019

the context solution would only work for renderers, internally we use formatter to support date/currency/etc display

@nihgwu nihgwu closed this Dec 9, 2019
@nihgwu nihgwu reopened this Dec 9, 2019
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.

2 participants