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 passing component name and props hash to data table cells #147

Conversation

andrewpye
Copy link
Member

Passing dynamic, data-driven components to polaris-data-table cells is currently very difficult owing to the impossibility of generating components for rendering outside template files. This PR aims to solve that issue by adding the ability to pass a hash with a componentName and optional props hash as cell data - the table will then render that component with the provided properties. This makes it possible to process e.g. an array of raw cell data in JS-land to instruct the table to render components in particular cells.

@andrewpye andrewpye self-assigned this Jul 2, 2018
@andrewpye andrewpye requested review from vladucu and tomnez July 2, 2018 09:56
Copy link
Member

@vladucu vladucu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks sane here

A few things come to my mind:

  1. Obviously we should look if we can include the template compiler with the addon
  2. Does the layout really need to be a CP? Wondering if this will be subject to double rendering issues
  3. Will this have issues accessing the Ember registry? Like if there's a service injected in the component that's passed in, will that work?

propsString: computed('propNames.[]', function() {
let propNames = this.get('propNames') || [];
return propNames.reduce((propsString, propName) => {
return `${propsString} ${ propName }=${ propName }`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some spacing inconsistencies around here

@andrewpye
Copy link
Member Author

andrewpye commented Jul 3, 2018

Obviously we should look if we can include the template compiler with the addon

Would be nice, but I'd prefer to avoid adding it unless it's needed... not sure how to handle this really at the moment, if you have any suggestions I'd appreciate it!

Does the layout really need to be a CP? Wondering if this will be subject to double rendering issues

I guess it's either a CP or gets explicitly set in didUpdateAttrs... didn't see any issues testing this but can push it harder to see if it breaks.

Will this have issues accessing the Ember registry? Like if there's a service injected in the component that's passed in, will that work?

Not entirely sure what you mean... this renders the "proxied" component the same way it'd get rendered by any other template, as far as I can tell. Can also test this before going any further.

@vladucu
Copy link
Member

vladucu commented Jul 3, 2018

Would be nice, but I'd prefer to avoid adding it unless it's needed... not sure how to handle this really at the moment, if you have any suggestions I'd appreciate it!

well, if we add this, would be a requirement no? yeah...will try to look into it a bit too

Not entirely sure what you mean... this renders the "proxied" component the same way it'd get rendered by any other template, as far as I can tell. Can also test this before going any further.

yeah, actually, I think you are right....but yeah, let's test in an app, to be sure

@andrewpye
Copy link
Member Author

andrewpye commented Jul 3, 2018

Would be nice, but I'd prefer to avoid adding it unless it's needed... not sure how to handle this really at the moment, if you have any suggestions I'd appreciate it!

well, if we add this, would be a requirement no? yeah...will try to look into it a bit too

I think it'd only error if a host app tries to use this new functionality - everything else should still be fine 🤞

@andrewpye andrewpye force-pushed the enhancement/allow-passing-component-props-to-data-table-cells branch from 0b23c4e to 7513065 Compare July 4, 2018 12:09
@andrewpye andrewpye force-pushed the enhancement/allow-passing-component-props-to-data-table-cells branch from 7513065 to 048fd97 Compare July 4, 2018 12:12
@andrewpye andrewpye merged commit 7b24927 into master Jul 4, 2018
@andrewpye andrewpye deleted the enhancement/allow-passing-component-props-to-data-table-cells branch July 4, 2018 12:35
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.

None yet

2 participants