Sortbydate #6214
Sortbydate #6214
Conversation
github-actions
bot
added
the
PR: waiting for review
Head branch was pushed to by a user without write access
|
o: it's been two weeks, hopefully this get reviewed soon so I can make any fixes necessary and/or start work on follow ups |
| return false | ||
| }).sort((a, b) => { |
There was a problem hiding this comment.
I haven't tested this yet and have not worked much in history.js, but from what I can tell, you are correct that this does seem to be something that should already be being handled by history.js and thus does not need any manual work being done here. If @absidue or @PikachuEXE have context on why this is being done here as well, we can add it back with a code comment containing that explanation, but this is fine from what I can see.
There was a problem hiding this comment.
It's from dbf69f2 (2 years ago)
Not sure if it's still needed
There was a problem hiding this comment.
I think it should be fine to remove it here, as the history store loads it sorted from the database and keeps the array in the right order when updating the history. The sort that I added for the history import in the store a few months ago is because the import replaces the array (as it has already merged the histories at the point) so it needs to be sorted before overwriting it.
| showLoadMoreButton: false, | ||
| query: '', | ||
| activeData: [], | ||
| } | ||
| }, | ||
| computed: { | ||
| historyCacheSorted: function () { | ||
| return this.$store.getters.getHistoryCacheSorted | ||
| return this.ascending ? [...this.$store.getters.getHistoryCacheSorted].reverse() : this.$store.getters.getHistoryCacheSorted |
There was a problem hiding this comment.
| return this.ascending ? [...this.$store.getters.getHistoryCacheSorted].reverse() : this.$store.getters.getHistoryCacheSorted | |
| return this.ascending ? this.$store.getters.getHistoryCacheSorted.toReversed(): this.$store.getters.getHistoryCacheSorted |
| @@ -44,14 +41,15 @@ export default defineComponent({ | |||
| dataLimit: 100, | |||
| searchDataLimit: 100, | |||
| doCaseSensitiveSearch: false, | |||
| ascending: false, | |||
There was a problem hiding this comment.
| ascending: false, | |
| useAscendingOrder: false, |
Recommend adjusting the variable name to be more specific.
| @@ -28,6 +28,13 @@ | |||
| :default-value="doCaseSensitiveSearch" | |||
| @change="doCaseSensitiveSearch = !doCaseSensitiveSearch" | |||
| /> | |||
| <ft-toggle-switch | |||
There was a problem hiding this comment.
suggestion (non-blocking): We do tend to use ft-selects for sort selections, and we may have more options for this in the future, so you may want to consider changing it. Not incredibly important at this point, though, aside from making the current labeling more likely to be temporary.
There was a problem hiding this comment.
Better have consistent UI unless there is a good reason
There was a problem hiding this comment.
ft-select will probably require me to make change to the store, not a problem, but I'm debating then, if I'm using ft-select I should stop using a boolean for this then, I didn't want to use an enum yet, since I don't even know if we will have sorting of anything besides date. (and if we are never sorting by anything else an enum feels unnecessary)
what are your thoughts on using an enum instead?
I can also add boolean as a type to ft-select, but I'm not sure what is the desired approach here
| @@ -269,6 +269,7 @@ History: | |||
| Empty Search Message: There are no videos in your history that match your search | |||
| Search bar placeholder: "Search in History" | |||
| Case Sensitive Search: Case Sensitive Search | |||
| Sort By Date ASC: Sort By Date ASC | |||
There was a problem hiding this comment.
issue: Related to the above, not a fan of this label, but hard to know of a better alternative without switching to ft-select. If you do do that, looking at the other sorting labels we have, more consistent ones would be DateWatchedOldest: Earliest Watched First and DateWatchedNewest: Latest Watched First
There was a problem hiding this comment.
I was also not a big fan of the label, but I also wasn't sure what to write, I'll look into ft-select and update the label accordingly (I like the Latest/Earliest watched first label more)
Sortbydate
Pull Request Type
Related issue
Initial addition to #5595 (not sure it would be considered closed and we just make a new issue for future changes)
Description
Adds a sort by date ascending/descending toggle to the history page, this is a stopgap to adding a more fully fledged filter by setting, but I wanted to keep the changes small and incremental since design wise, some decisions have to be made and I'm not sure I'm the one that should make those decisions. (like other kinds of sorting/filtering in history page)
I also removed the second sorting it does when it does the search, since the array it uses is already sorted (in whichever order we choose), and the filter method I believe does a linear scan, it seemed unnecessary to sort twice, if we think it's necessary I can add it back and just add a condition to sort whichever way we need it to (But I don't think it's needed)
Screenshots
Testing
I tested by adding random videos to history, sorting it back and forth, then doing search and sorting back and forth as well
possibly with a huge history we'd like to view how the sorting performs... I don't have a huge history at the moment, so it's hard to tell. it's expected to a certain degree
Desktop
Additional context
I stuck to the simple toggle since I was only going to do the asc/desc sorting on this PR