light-table: add iconSortable property
#464
Conversation
There was a problem hiding this comment.
There's a typo at @property iconDescending.
Other than that, great PR! ❤️
I'd still like to wait for @alexander-alvarez' opinion on my other comments.
addon/mixins/table-header.js
Outdated
| * | ||
| * @property iconDescending | ||
| * @property iconDesci |
addon/components/columns/base.js
Outdated
| } | ||
|
|
||
| return 'iconSortable'; | ||
| }), |
There was a problem hiding this comment.
/**
* @property sortIcon
* @type {String|null}
* @private
*/
sortIcon: computed('column.{sortable,sorted,ascending}', function() {
let sorted = this.get('column.sorted');
if (sorted) {
let ascending = this.get('column.ascending');
return ascending ? 'iconAscending' : 'iconDescending';
}
let sortable = this.get('column.sortable');
return sortable ? 'iconSortable' : null;
}),I think I would rather mark sortIcon as private (for now). And only return a sortIcon, if it was set. That way later down in the template, we can just call {{if sortIcon}} as to not insert a "meaningless" <i> tag.
This behavior is slightly different than the current one, but I think this makes more sense. I doubt that the "empty" <i> tag is used in the wild.
Also, brace expansion, yay! 🎉
@alexander-alvarez What is your opinion?
There was a problem hiding this comment.
Sounds good.
The only thing I can add is a 🚲 🏡 on the name -- sortIconProperty or sortIconAttribute just so it's clear that this string represents the attribute that will be pulled from to generate the icon simply from the name (we could add docs for it too)
| {{#if column.sorted}} | ||
| <i class="lt-sort-icon {{if column.ascending sortIcons.iconAscending sortIcons.iconDescending}}"></i> | ||
| {{#if column.sortable}} | ||
| <i class="lt-sort-icon {{get sortIcons sortIcon}}"></i> |
There was a problem hiding this comment.
With the other comment regarding sortIcon in mind, this would then be:
|
Don't get thrown off by the missing-tests label. We didn't have any in the first place. 😜 If you want, you can add some. If not, I'll do it afterwards. 😉 |
|
@buschtoens I noticed there weren't any tests explicitly for the sort icons. I would be up for taking a crack at putting in some integration tests! |
|
I'd love that. |
ignatius-j
changed the title
iconSortable property
|
@buschtoens @alexander-alvarez I fixed the comments and added the initial integration test for making sure the sort icons render properly on Just let me know what you think! |
| let sortIcon = find('.lt-sort-icon', sortableHeader); | ||
|
|
||
| // Sortable case | ||
| assert.ok(hasClass(sortIcon, 'fa-sort'), 'Sortable icon renders'); |
There was a problem hiding this comment.
good test cases! I think it's fine as 1.
this also brought to light for me that the default behavior when sortable is enabled is to show the sortable icons.
If someone is expecting/want it not to show up it could be seen as a "breaking" change that the icon now show up.
Do we care? Should we document how turn it off? Have a cookbook example where it only shows on hover? Have a showSortableIconByDefault configuration option at the ember-cli-build.js level?
There was a problem hiding this comment.
Before, if the column was sortable but hadn't been sorted yet, it would display the icon in the DOM but with just the lt-sort-icon class and no other classes (<i class="lt-sort-icon"></i>).
To get that same result now, you would just not pass in any iconSortable to lt-head (because by default iconSortable is ''). So it should probably be stated somewhere that it is an optional field that you shouldn't pass in if you don't want an icon for flagging that the column is sortable.
I could also try to put together a Cookbook example of an iconSortable that only shows on hover. Let me know if you think that would be helpful!
There was a problem hiding this comment.
woops 🚪 🚶♂️ didn't pay close enough attention to the default.
you're right.
Closes #463