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/custom icon components #489

Merged
merged 11 commits into from
Oct 26, 2017
Merged

Feat/custom icon components #489

merged 11 commits into from
Oct 26, 2017

Conversation

RedTn
Copy link
Contributor

@RedTn RedTn commented Oct 9, 2017

This a a PR for an issue I submitted for customizable sort components: #478

@alexander-alvarez
Copy link
Collaborator

@RedTn sorry for the delay in communication. I had a weird week with holidays, vacation, and other stuff...
Will try to get to it early this week, but on first glance looks good.

@RedTn
Copy link
Contributor Author

RedTn commented Oct 16, 2017

Feel free to let me know if anything needs to be changed/updated

@RedTn
Copy link
Contributor Author

RedTn commented Oct 23, 2017

@alexander-alvarez Checking up on this PR, thanks alexander!

Copy link
Collaborator

@alexander-alvarez alexander-alvarez left a comment

Choose a reason for hiding this comment

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

A testing request and a comment on the docs. Otherwise looks great.
Sorry for the delay.

@@ -2,7 +2,9 @@
{{component column.component column=column table=table tableActions=tableActions extra=extra sortIcons=sortIcons}}
{{else}}
{{label}}
{{#if sortIconProperty}}
{{#if (and sortIcons.iconComponent sortIconProperty)}}
{{component sortIcons.iconComponent sortIcons=sortIcons sortIconProperty=sortIconProperty}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

would you mind adding a test to verify the arguments (sortIcons=sortIcons sortIconProperty=sortIconProperty) get passed to the component declared by iconComponent when we render a table like you do in the demo app:

+  {{t.head
+    onColumnClick=(action 'onColumnClick')
+    iconSortable='unfold_more'
+    iconAscending='keyboard_arrow_up'
+    iconDescending='keyboard_arrow_down'
+    iconComponent='materialize-icon'
+    fixed=true
+  }}

You can steal some ideas from
https://github.com/cibernox/ember-power-select/blob/master/tests/integration/components/power-select/customization-with-components-test.js#L257..L281

@@ -107,6 +107,17 @@ export default Mixin.create({
iconDescending: '',

/**
* See `iconSortable, iconAsending, or iconDescending. Custom sorting component
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change the order

Custom sorting component name to use instead of the default `<i class="lt-sort-icon"></i>` template. 
See `iconSortable`, `iconAsending`, or `iconDescending` for more info.

Uses the same properties.

It's unclear what you mean by this sure what you mean by this

@@ -92,6 +94,34 @@ test('render sort icons', function(assert) {
assert.notOk(hasClass(sortIcon, 'fa-sort-asc'));
});

test('custom iconComponent has arguments', function(assert) {
const iconSortable = 'unfold_more';
Copy link
Collaborator

Choose a reason for hiding this comment

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

assert.expect(6)
otherwise the test will pass if the component doesn't get rendered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexander-alvarez Will add this soon, thanks alexander I'm learning quite a bit from this PR

@@ -107,6 +107,15 @@ export default Mixin.create({
iconDescending: '',

/**
* Custom sorting component name to use instead of the default `<i class="lt-sort-icon"></i>` template.
* See `iconSortable, iconAsending, or iconDescending.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor formatting error with the backticks here.

See `iconSortable`, `iconAsending`, or `iconDescending`.

@@ -2,7 +2,9 @@
{{component column.component column=column table=table tableActions=tableActions extra=extra sortIcons=sortIcons}}
{{else}}
{{label}}
{{#if sortIconProperty}}
{{#if (and sortIcons.iconComponent sortIconProperty)}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't remember, if we talked about that. but this way the iconComponent would only get rendered when there is a sortIconProperty, i.e. when either of sortable or sorted is true.

I don't have strong feelings either way, since I can't come up with a scenario where you would want to render an iconComponent when you don't have any icons to display. Just wanted to have mentioned it. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember I ran into this issue in the demo where the avatar column was displaying the sort component when sortable: 'false'.
Because the iconComponent is being specified in the hbs file on the table, all the columns will utilize the iconComponent, so we need a check if the user doesn't want a column to be sortable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, but some users might argue that they'd want their iconComponent to display a "not sortable" icon and that it's up to the user to handle the "not sortable" state instead of ELT doing that for the user.

For instance:

{{#if sortIconProperty}}
  <i class="lt-sort-icon material-icons" style="height:0px;">{{get sortIcons sortIconProperty}}</i>
{{else}}
  <i class="lt-sort-icon material-icons" style="height:0px; color: red;">not_sortable</i>
{{/if}}

Copy link
Contributor Author

@RedTn RedTn Oct 24, 2017

Choose a reason for hiding this comment

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

Understood, I see the use case now. Because you have no strong feelings I guess I'll leave the base.hbs as is for now but feel free to let me know otherwise.

@buschtoens buschtoens merged commit e16af17 into adopted-ember-addons:master Oct 26, 2017
@buschtoens
Copy link
Collaborator

Thanks so much @RedTn ❤️

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

7 participants