Implement sorting menu #156
Conversation
|
Just noticed that unlike Release or Last Updated, Last Played should provide a strict total ordering and consequently needs to take time into account, even if the time probably won't be user-facing. I'll work on it... |
|
Since I wasn't sure if you'd like using "Today"/"Yesterday" for dates in mod details, I made a gist of the changes: https://gist.github.com/jg18/11844ab6af10b53f911db4edaee56111 |
There was a problem hiding this comment.
Phew, lots of changes. Looks good overall, although I'm not happy with the way last_played is handled. Take a look at InstalledMod.save and InstalledMod.save_user to get an idea of how existing user-specific stuff is saved. I don't like adding new parameters to get() since modifications to the mod data for displaying purposes (or similar stuff) should be handled in the same location where the list is generated since that's usually closer to the related code.
| @@ -21,7 +24,7 @@ export default { | |||
|
|
|||
| search_text: '', | |||
| status_message: '', | |||
| tab: 'explore', | |||
| tab: 'explore', // FIXME shouldn't this be initialized to 'home'? isn't that what's assumed elsewhere like in center? | |||
There was a problem hiding this comment.
Doesn't really matter since it's overwritten on initialisation anyway.
There was a problem hiding this comment.
When 'explore' is the initial value, on startup the Explore tab header is briefly highlighted before the highlighting moves to the Home tab header. If the initial value is 'home' then that doesn't occur. Ultra-tiny nitpickI admit. Opinion?
There was a problem hiding this comment.
That's... a good reason to change it. IIRC explore used to be the default tab which later changed due to user feedback and the default value here is just a relic. Feel free to change it.
|
Thanks for the feedback. Yeah some of these changes (especially the |
|
Tangentially, since the Reminds me also that now that the welcome page changes are in, we need a commit to start adding the |
|
I think the new commit addresses all of your comments. Thanks for the thorough review. Once all of my other PRs are merged, I'll do an interactive rebase of this branch, dropping the commits copied over from |
|
LGTM. Do you want to do the default value change for |
Note: this is a WIP with a bunch of known issues.
This branch is based on the branch for PR #155 since delta updates for mod list updates are required to get reasonable responsiveness.
I couldn't get the existing sorting menu to work (event listeners wouldn't fire no matter what I did) so I switched to useing popper.js since we already use it in the Home tab. Hope it can be made to work here...
Known issues:
The "Last Played" option is missing because it doesn't work yet.Fixed!The formatting and styling are all messed up. Presumably fixing this requires the right mix of changes to CSS and popper.js options, but my CSS is kind of weak, so I'd really appreciate help here.Fixed! Formatting/styling look decent to me now although you be the judge. 😜The sort buttons are only clickable from a small spot on the right edge of the button. Not sure why that is.Fixed!The sorting menu icon button still looks active even after the menu is closed sometimes.Fixed!Switching between Home and Explore tabs causes sorting menu selection to be lost.Apparently this is no longer an issue.