Skip to content

Sort by Track#273

Merged
phanan merged 2 commits intokoel:masterfrom
alex-phillips:feature/sort-by-track
Mar 20, 2016
Merged

Sort by Track#273
phanan merged 2 commits intokoel:masterfrom
alex-phillips:feature/sort-by-track

Conversation

@alex-phillips
Copy link
Copy Markdown
Contributor

This adds an optional subSortKey argument to the sort function so that we can perform a subsort on the data structure to allow for sorting by tracks when we sort by album.

This address issue #240

@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

Thanks, but your PR contains a number of problems:

  • Code style (as specified by CI)
  • Uses var instead of let or const
  • Contains logging code
  • Addesses multiple functionalities in one PR
  • Commits not squashed
  • Most importantly, missing functionality: If track is introduced, shouldn't it be editable?

@phanan phanan closed this Mar 20, 2016
@phanan phanan reopened this Mar 20, 2016
@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

My bad – didn't notice you submitted another PR for the extra functionality. Do you mind fixing the other issues listed above though? Thanks!

@alex-phillips alex-phillips force-pushed the feature/sort-by-track branch from dd09082 to 6bdafaa Compare March 20, 2016 12:58
@alex-phillips
Copy link
Copy Markdown
Contributor Author

@phanan fixed all of the above including added support for editing the track in the modal

with a second sort key. track numbers are also editable via the song
edit modal interface.
@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

That's fast! Please give me some time to validate the changes.

@alex-phillips alex-phillips force-pushed the feature/sort-by-track branch from 6bdafaa to 940cd1a Compare March 20, 2016 13:07
@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

Hmm… Actually, this PR breaks the tests.

@alex-phillips
Copy link
Copy Markdown
Contributor Author

No problem. Forgot to consider the new column track in the tests. Should be all fixed now.

There might be a better way to implement the sub sort key - maybe a recursive function so that you can pass an arbitrary number of subsort keys and it would traverse all of them? I'm still fairly new to Vue JS so not sure how to call that sort function inside of itself since it is a Vue.filter definition.

@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

Come to think of it, should we implement this sub-sort feature though? I'd suggest to keep it as simple as an extra standalone Track # column, à la iTunes:

image

Granted, this column isn't even helpful in a "mixed" screen, but shouldn't the users only use it for Single Album view, where it's meant to be used?

If you chose to go this route, please take note of the responsive views.

@alex-phillips
Copy link
Copy Markdown
Contributor Author

I'm a little confused. As far as I can tell, iTunes does sort your library based on album groupings as well as track number. Ex: if you view your entire library and sort by the "Artist" column, the artists are group but sub-sorted alphabetically by their album names and the albums are sub-sorted by the tracks so that the album stays in the proper order. We could add a column like you said, but I think sorting on track number is extremely important.

As I mentioned above, this should probably be taken a step further where we can pass in an arbitrary number of sub sort keys so that you can have it perform like iTunes does (sort by Artist name, then album, and then by track within that album).

I'd be happy to tackle this arbitrary number of sorts, but as I said I'm not as familiar with Vue.js quite yet. Not sure how to reference the sort function within itself.

@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

I've just checked iTunes again – the behavior is exactly like what you said. Man I really need some coffee.

@alex-phillips
Copy link
Copy Markdown
Contributor Author

Haha, no worries. I'm going to see if I can tackle that advanced subsorting I mentioned. Feel free to merge this if you want, but I may open another PR to implement further sorting.

phanan added a commit that referenced this pull request Mar 20, 2016
@phanan phanan merged commit 1207ea2 into koel:master Mar 20, 2016
@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

❤️

@phanan
Copy link
Copy Markdown
Member

phanan commented Mar 20, 2016

Please use php artisan make:migration for any database schema changes instead of modifying the original files.

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.

2 participants