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

Customizable components #177

Merged
merged 7 commits into from
Sep 21, 2016
Merged

Customizable components #177

merged 7 commits into from
Sep 21, 2016

Conversation

taras
Copy link
Collaborator

@taras taras commented Sep 16, 2016

This feature makes it possible to provide custom lt-* components for the body of the table and customize the behaviour on per table basis.

For example:

{{#light-table table as |t|}}
  {{t.body 
     rowComponent=(component "active-row" classNames="active-row" active=activeItem)
  }}
{{/light-table}}

/components/active-row.js

import Ember from 'ember';
import RowComponent from 'ember-light-table/components/row';

export default RowComponent.extend({
  classNameBindings: ['isActive'],
  isActive: Ember.computed('active', 'row.content', function() {
    return this.get('row.content') === this.get('active');
  })
})

@offirgolan
Copy link
Collaborator

@taras this looks good to me. I have one suggestion though. From my experience, many users just have a single component definition which get different table instances passed in. Would it be simple to do something like the following:

new Table(column, rows, {
  rowComponent: 'myTableRow',
  spannedRowComponent: 'mySpannedRow'
});

@taras
Copy link
Collaborator Author

taras commented Sep 19, 2016

I don't think we should be mixing the state and renderer.

We can do something like this:

{{light-table table rowComponent=(component 'my-table-row')}}

@offirgolan
Copy link
Collaborator

Although true, whats the difference between specifying a cellComponent and a rowComponent? Ideally, its the same concept. I just think that at this point, we are overloading the template. If you pass a component name via the table instance for rowComponent, all thats really needed inside that component is the table instance, the columns, and the actual row content. Everything else such as class names, bindings, etc, can be done on the component itself.

@taras
Copy link
Collaborator Author

taras commented Sep 19, 2016

@offirgolan the same thing applies to cellComponent but cellComponent is existing API.

Passing cellComponent as a string is limiting API because it doesn't allow you to pass a closure component. Doing what I'm doing with rowComponent is not possible with the current implementation of cellComponent.

With that said, it would be difficult to implement cellComponent in the same way as rowComponent because there is no convention for creating columns in the template.

@taras
Copy link
Collaborator Author

taras commented Sep 19, 2016

new Table(column, rows, {
  rowComponent: 'myTableRow',
  spannedRowComponent: 'mySpannedRow'
});

I'm not really sure what this offers. Why do you think this is a good idea?

@offirgolan
Copy link
Collaborator

@taras

new Table(column, rows, {
  rowComponent: 'myTableRow',
  spannedRowComponent: 'mySpannedRow'
});

I like this approach because of the following:

  1. This allows you to declare a single table component that is fully dependent on the passed table instance which can also allow such declaration to be modular.
  2. This keeps consistent with the current API. Currently, all custom table rendering is done via passed in table column properties. Your implementation now splits this declaration so you have to declare which components you want to use in multiple places.

As to what you were saying, I 100% understand your direction and I like as to where it's going but I dont feel like it is the appropriate time to change the structure / direction of the API. I think that we should either make all rendering decisions on the component declaration side OR on the table declaration side. Not both.

@taras
Copy link
Collaborator Author

taras commented Sep 19, 2016

These 2 implementations are not actually equivalent. They're not just different APIs, they're actually different in their capability.

Closure component API allows to dynamically update the row when bound data changes. This is not possible with component string name API.

@offirgolan
Copy link
Collaborator

Yeah I guess that's true. Lets go ahead with this implementation. Hopefully in the near future we can delegate all renderer tasks to the template instead of the table instance (i.e. ember-microstates 😜).

Once documentation is added to the possible t.body components, feel free to merge!

@taras
Copy link
Collaborator Author

taras commented Sep 19, 2016

@offirgolan do you want me to add the {{light-table table rowComponent=(component 'my-row')}}?

@taras taras merged commit 5ebc871 into master Sep 21, 2016
@taras taras deleted the customizable-components branch September 21, 2016 19:26
offirgolan added a commit that referenced this pull request Sep 25, 2016
* master: (28 commits)
  Use ember-cli-code-coverage for code coverage (#185)
  Removed ember-cli-blanket (#176)
  chore(package): update ember-get-config to version 0.1.8 (#184)
  Add scaffolding comment
  Released v1.4.0
  Add footer scaffolding and move width into style attr (#183)
  chore(package): update ember-cli-jshint to version 2.0.1 (#170)
  chore(package): update ember-code-snippet to version 1.6.0 (#174)
  chore(package): update ember-cli-mirage to version 0.2.2 (#180)
  Customizable components (#177)
  Revert "Move scaffolding width to style attribute"
  Move scaffolding width to style attribute
  Update component usage docs
  Demo and Docs modifications (#171)
  Fix docs typo
  [FEATURE] Two-way sync between rows and model (#167)
  Update ember-cli to version 2.8.0 🚀 (#165)
  chore(package): update ember-cli-qunit to version 3.0.1 (#168)
  Released v1.3.1
  [FEATURE] Introduce `resizeOnDrag` for column resizing (#166)
  ...

# Conflicts:
#	addon/classes/Table.js
#	addon/components/cells/base.js
#	addon/components/columns/base.js
#	addon/templates/components/lt-head.hbs
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