-
Notifications
You must be signed in to change notification settings - Fork 10
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(inline recs): Order tracks by ascending release dates #764
feat(inline recs): Order tracks by ascending release dates #764
Conversation
Hmm. |
b660bda
to
613209f
Compare
Super! I managed to repair my stuff, thanks to a PC with TortoiseGit (requires admin rights). |
Ah, I should also add medium and track in the sort.
We see they are in random order twice: 1.8, 1.9 (good) but then 1.9, 1.8 (no good). |
c74a5a6
to
79fa5ab
Compare
Moved to #777 |
Display total tracks in tooltip Cosmetic fixes
79fa5ab
to
6fe3015
Compare
I renamed my commits to pass that test. |
Moved to #776 |
Maybe you'll prefer 3 separate PR. |
63e1e22
to
707b4d6
Compare
For sake of clarity, I have split this PR in 3: |
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 feel that it is a bit too hacky to inject an HTML comment into the DOM just for sorting purposes. It would be cleaner to sort the releases with a custom sorting function before mapping them to HTML with formatRow
.
P.S. Since I don't have permissions to push directly to the main
branch, you can also re-add the version bump commit to this PR.
Ok, I will try to do that! 😉 |
Instead of HTML comment sorting hack Improves ROpdebee#764 with this @kellnerd advice
3ce288b
to
267e206
Compare
You were right, @kellnerd: With the sorting function, the code looks cleaner! |
Would you like that I squash this into a single commit, before merge? |
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.
Thank you @jesus2099 for this and the other PRs, and thanks @kellnerd for reviewing and processing them while I'm pre-occupied! ❤️
Just a small nitpick: Could you also change the month in the version number? Other than that, LGTM.
@jesus2099 Don't bother squashing the commits, we'll do that when merging. 🙂
Instead of HTML comment sorting hack Improves ROpdebee#764 with this @kellnerd advice
267e206
to
ede535a
Compare
Oh you're right, I got the wrong date! |
For the moment, release tracks were displayed in random order.
Now they will be ordered by date, then title, then comment.
I have also added display of release comment and added release date as tooltip.
@ROpdebee, may I leave you manage the version number, OK?
My 3 concurrent PR have no conflict between themselves: