search

custom-cards / flex-table-card

Highly Flexible Lovelace Card - arbitrary contents/columns/rows, regex matched, perfect to show appdaemon created content and anything breaking out of the entity_id + attributes concept
GNU General Public License v3.0
198 stars 23 forks source link

feat-sort-by-col-header #107

Closed EdLeckert closed 9 months ago

EdLeckert commented 10 months ago

Satisfies feature request:

After initial sort from config, columns can be sorted ascending/descending by clicking on column header. Order of other columns then becomes indeterminate.

Took the suggestion by @myhomeiot to use ZHA Network Card as an example. Code has diverged considerably since the original branch, but with modifications it was quite useful.

When sorting via config, the user is responsible for making certain that any sort_by columns are configured properly; e.g., if an id option is required because the data option of all columns is identical, then the user must provide an id option for the sortable columns. However, in the case of column sorting, all visible columns must be sortable without any assistance from the user. For this reason, I have added the name option as a value that could be used in the sort_by list. While the name option is not currently listed as being required by the config for columns, it is probably the most likely to be available consistently. It has been placed last in the list, following such options as id and data.

Would it be possible to make name a required option for columns going forward? It's a breaking change, but a simple one, and probably won't affect many users. In fact, I don't see why the name value wouldn't be the preferred way to specify the sort_by option, essentially rendering the id field obsolete. It's quite useful if not necessary for naming columns, and should always be unique, I would think. Am I missing something?

Anyway, this could use some testing with a wider variety of configurations than I can dream up, but I know of no issues other than a dependency on the name option.

EdLeckert commented 9 months ago

Made one more change to provide a calculated default for a missing name option, so it doesn't need to be provided and the manual sort will still work.

Wasn't sure about the col_ids restriction due to having an icon, so removed it. It was undefined in all of my cases anyway. Let me know if this is a problem.

I really do believe that encouraging the use of a name option on each column, even if it's not required, is the right thing to do. And the id column adds a small bit of confusion that I think we could do without in the future, simply by encouraging the use of name instead.

Thoughts?

daringer commented 9 months ago

lgtm, personally I would not be in favor of requiring name as this will likely break some existing setups. Apart from that this looks good, merging...

PeteRager commented 9 months ago

This is a super awesome enhancement. Will this get pushed in a release in the next days or should I just clone master?

EdLeckert commented 8 months ago

@daringer: Any chance of cutting a new release? I'm particularly interested in seeing #105 get released.

daringer commented 7 months ago

yeah, good point, will try to do it today - sry for the delays

daringer commented 7 months ago

@EdLeckert done, testing looks good for me

EdLeckert commented 7 months ago

@daringer Thanks!