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-sort-by-col-header #107

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

EdLeckert
Copy link
Contributor

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
Copy link
Contributor Author

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?

@EdLeckert EdLeckert marked this pull request as draft September 16, 2023 22:17
@EdLeckert EdLeckert marked this pull request as ready for review September 16, 2023 23:03
@daringer
Copy link
Collaborator

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...

@daringer daringer merged commit 7ad6ebf into custom-cards:master Sep 19, 2023
1 check passed
@EdLeckert EdLeckert deleted the feat-sort-by-col-header branch September 19, 2023 15:32
@PeteRager
Copy link

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

@EdLeckert
Copy link
Contributor Author

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

@daringer
Copy link
Collaborator

daringer commented Nov 6, 2023

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

@daringer
Copy link
Collaborator

daringer commented Nov 6, 2023

@EdLeckert done, testing looks good for me

@EdLeckert
Copy link
Contributor Author

@daringer Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants