UI: Fix client sorting #6817
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import Mixin from '@ember/object/mixin'; | ||
| import Ember from 'ember'; | ||
| import { computed } from '@ember/object'; | ||
| import { warn } from '@ember/debug'; | ||
|
|
||
| /** | ||
| Sortable mixin factory | ||
|
|
||
| Simple sorting behavior for a list of objects. Pass the list of properties | ||
| you want the list to be live-sorted based on, or use the generic sortable.js | ||
| if you don’t need that. | ||
|
|
||
| Properties to override: | ||
| - sortProperty: the property to sort by | ||
| - sortDescending: when true, the list is reversed | ||
| - listToSort: the list of objects to sort | ||
|
|
||
| Properties provided: | ||
| - listSorted: a copy of listToSort that has been sorted | ||
| */ | ||
| export default function sortableFactory(properties, fromSortableMixin) { | ||
| const eachProperties = properties.map(property => `listToSort.@each.${property}`); | ||
|
|
||
| return Mixin.create({ | ||
| // Override in mixin consumer | ||
| sortProperty: null, | ||
| sortDescending: true, | ||
| listToSort: computed(() => []), | ||
|
|
||
| _sortableFactoryWarningPrinted: false, | ||
|
|
||
| listSorted: computed( | ||
| ...eachProperties, | ||
| 'listToSort.[]', | ||
| 'sortProperty', | ||
| 'sortDescending', | ||
| function() { | ||
| if (!this._sortableFactoryWarningPrinted && !Ember.testing) { | ||
| let message = | ||
| 'Using SortableFactory without property keys means the list will only sort when the members change, not when any of their properties change.'; | ||
|
|
||
| if (fromSortableMixin) { | ||
| message += ' The Sortable mixin is deprecated in favor of SortableFactory.'; | ||
| } | ||
|
|
||
| warn(message, properties.length > 0, { id: 'nomad.no-sortable-properties' }); | ||
| this.set('_sortableFactoryWarningPrinted', true); | ||
| } | ||
|
|
||
| const sorted = this.listToSort.compact().sortBy(this.sortProperty); | ||
| if (this.sortDescending) { | ||
| return sorted.reverse(); | ||
| } | ||
| return sorted; | ||
| } | ||
| ), | ||
| }); | ||
backspace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,5 @@ | ||
| import SortableFactory from 'nomad-ui/mixins/sortable-factory'; | ||
|
|
||
| // A generic version of SortableFactory with no sort property dependent keys. | ||
|
|
||
| export default SortableFactory([], true); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,7 +61,13 @@ export default Model.extend({ | |
|
|
||
| // A status attribute that includes states not included in node status. | ||
| // Useful for coloring and sorting nodes | ||
| compositeStatus: computed('isDraining', 'isEligible', 'status', function() { | ||
| if (this.isDraining) { | ||
| return 'draining'; | ||
| } else if (!this.isEligible) { | ||
| return 'ineligible'; | ||
| } else { | ||
| return this.status; | ||
| } | ||
| }), | ||
|
There was a problem hiding this comment. Hmmmm. I think we can drop There was a problem hiding this comment. Agreed, I did this in ad3863f. One caveat is that I removed an assertion in a client detail test: Being able to test that with randomness was unreliable and would require the duplication of the I did make Also, I’ve been surprised that hovering over the status light doesn’t have any description of what the colour means; it’s true that it can usually be decoded by looking for the corresponding colour in the client details boxed section, but maybe a tooltip would be a good idea to be more explicit? There was a problem hiding this comment. Good thoughts on ways to improve that indicator. I know @joshklekamp noodled with it too as part of the drain work (added icons, thought through colors). I forget if he added a draining state. Either way, worth discussing further but out of scope for this bug fix. |
||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,8 @@ $size: 0.75em; | |
| ); | ||
| } | ||
|
|
||
| &.ineligible, | ||
| &.draining { | ||
| background: $warning; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your concern that this problem should be fixed for all tables, and I also agree your resistance to dig into all of them at once.
Maybe a good middle ground just so the problem isn't forgotten about once this merges is to
warnhere ifproperties.length === 0. The warning could also include something like "The Sortable mixin is deprecated in favor of SortableFactory".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for the suggestion; I added something in 0131c36, it’s somewhat unwieldy because of when mixins are mixed in, but it works, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it'll do, but I agree it's unwieldy. What's preventing you from putting it right here, in the
sortableFactorybut not in the returnedMixin? Don't we have all the information at this point? It would also prevent the state tracking needed right now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, I merged this 😳 I’m sorry
I think that would have the same problem though because the function is called when the mixin is created, which happens when the application loads, so the warnings all happen at the same time.