Skip to content

Add UI for sorting my crates by recent downloads #1232

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

Merged
merged 2 commits into from
Jan 17, 2018
Merged

Add UI for sorting my crates by recent downloads #1232

merged 2 commits into from
Jan 17, 2018

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jan 13, 2018

This fixes #1231, and renames "Downloads" to "All-Time Downloads" to be consistent with the sort dropdowns on other listings.

This fixes #1231, and renames "Downloads" to "All-Time Downloads" to be consistent with the sort dropdowns on other listings.
@carols10cents
Copy link
Member

Thanks! I think this is almost there. I know this is hard to test because there isn't a way to log in when you're running npm run start:live, so you have to set up a backend and all that.

But when I was running this PR locally, when I go to /me/crates and then switch the sort to "All-Time Downloads", the closed dropdown says "Downloads", and if I switch to "Recent Downloads" the closed dropdown says "Alphabetical".

That's because the closed dropdown displays the value in currentSortBy, which comes from this function in controllers/me/crates.js, and that function only knows about "Downloads" and "Alphabetical". That function needs to change to look more like the other controllers, such as controllers/crates.js.

We're likely doing something wrong to end up with all this duplication (and it's been this way for a while), but I'm not sure the best way to fix the problem especially because while most of the sort dropdowns are the same, they aren't all the same (search result lists have "relevance" as an option).

So would you be willing to make the easy fix of updating the currentSortBy function in app/controllers/me/crates.js? Or would you like me to take care of it? It's no problem, just let me know!

@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 14, 2018

Fixed in a1eacf4! Thanks for the detailed instructions :)

Wouldn't it be possible to have the same sort be used everywhere, and then override it specifically on the search results page (which should presumably be the only one that differs)?

@carols10cents
Copy link
Member

Thank you, this works great now!!

bors: r+

Wouldn't it be possible to have the same sort be used everywhere, and then override it specifically on the search results page (which should presumably be the only one that differs)?

Yep, it's certainly possible, and doing so would fix #9. There might be other places that are different, I'm not sure!

bors-voyager bot added a commit that referenced this pull request Jan 17, 2018
1232: Add UI for sorting my crates by recent downloads r=carols10cents

This fixes #1231, and renames "Downloads" to "All-Time Downloads" to be consistent with the sort dropdowns on other listings.
@bors-voyager
Copy link
Contributor

bors-voyager bot commented Jan 17, 2018

Build succeeded

@bors-voyager bors-voyager bot merged commit a1eacf4 into rust-lang:master Jan 17, 2018
@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 23, 2018

The waiting-on-author label should probably be removed. Also, how/when do merged PRs show up on crates.io?

@carols10cents
Copy link
Member

Just deployed, this is live now :)

I don't usually bother removing status labels from merged PRs, I only use those when looking at open PRs, so the label hanging around is fine :)

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.

Sort "my crates" by recent downloads
2 participants