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

change the property type of column className and cellClassName to be String only #573

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

Conversation

baljeet03
Copy link

This is regarding the below issue.
#416

That's my first PR in open source community :)

@alexander-alvarez
Copy link
Collaborator

Hi @baljeet03 I'm so sorry for not following up sooner.
I'm actually not sure about #416, so I'd need to review with @buschtoens
https://github.com/emberjs/ember.js/blob/v3.3.1/packages/ember-views/lib/mixins/class_names_support.js#L32 is an array, so as is should work.

@baljeet03
Copy link
Author

baljeet03 commented Aug 2, 2018

@alexander-alvarez Though ember support it, we are adding this in classBindings and the passed array is being joined by comma , rather than space . So it will not work. One solution is to make this as computed property which will resolve this issue or we could just send as string only. I am open to both but will prefer computed one. I am open to suggestion :)

@alexander-alvarez
Copy link
Collaborator

Ooooh.. I see what you mean now, thanks for the clarification.

https://github.com/offirgolan/ember-light-table/blob/6ee059272b85b7efd2ff5acdead16ac00b8b4461/addon/components/columns/base.js#L23
https://github.com/offirgolan/ember-light-table/blob/v1.8.6/addon/components/cells/base.js#L25
https://github.com/offirgolan/ember-light-table/blob/6ee059272b85b7efd2ff5acdead16ac00b8b4461/addon/components/lt-row.js#L9

Like you mention, I think the computed property one would be best. Make sure to add tests so that we can make sure this works for the case as a string or as an array!

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