Sort by group size (Mentioned in #306). Popular groups sortBy -size #335
Conversation
…By -size Will lazily update over 2h after pushing. Linkifies Popular Group heading. Depends on OpenUserJS#332 for CSS.
|
Ascending/descending doesn't appear to be linked in... EDIT: this definitely needs to be implemented though. |
It's currently populating Group.size when it's updates Group.rating every 2 hours. We could easily populate all groups but that requires access to the db... I could create a route to force updating all groups I guess. |
|
@Martii commented on 28 aug. 2014 05:44 CEST:
@Zren Have a look at this PR how to include ascending/descending ordering. Link to #306 |
|
I had already implemented it @jerone https://github.com/OpenUserJs/OpenUserJS.org/pull/335/files#diff-e731f3e65d80f95c77cf8955531667f6R171 It's just that the we aren't migrating all at once like normal. Know what. We need a new issue to deal with this. |
What do you mean by this? (#337 referenced). The PR READY label has to be done... you didn't tag the remaining yesterday so they aren't merged and especially the needs discussion label that was there on a few means it's not ready. e.g. blocking by you as the assignee... Did I miss your point here? |
|
This PR appears to have a CSS/HTML artifact for "Popular Groups" that isn't present on current deployed HEAD at 8f951b1. Unmarking as PR READY. Remark when PR READY. Thanks. |
|
This PR also appears to break #332. Re:
It would appear that Zren is forcing commits into the project. When I test his individual commits this is when breakage occurs. This latest one appears to be reliant, and undocumented, upon #333 in order to work. This is a bad habit to get into especially since neither issue topic should be related. I do understand that some PRs have the need for dependencies... but a forced feature is not a good thing especially when undocumented. Unmarked PR READY on both of these until they can be isolated for testing. Hope that clears up some confusion as to the earlier report I did and you responded to. |
|
@Zren commented on 28 aug. 2014 22:26 CEST:
@Martii commented on 2 sep. 2014 09:36 CEST:
It appears this has been correctly implemented by @Zren. I was confused by the new link in the |
...
I assume we're keeping the default sort order on the group page, which is why we change it here.
In other words, you're not actually using the latest head for testing? Did you test with after doing a |
In GitHubs words the latest HEAD is 8f951b1 at the top summary commit. I always pull from upstream master... specifically I fetch from remote upstream, merge upstream/master if available into my master, then checkout your pr which will be updated or not depending on what your pr head is at. |
|
Sometimes I will also build a testing branch of all the available commits exactly in order which is what I did to find what didn't appear to be a boog. I'll recheck merging master into the pr. |
|
Rechecked and yes the current HEAD wasn't in the pr checkout. This was the first time I've done it (#334) out of order and it (remerging master into the pr) does negate the appearance of a boog. Anyhow... going back to linear merging, to avoid this procedure in the future, I would like to see #333 cleared up so we can do this one. |
Sort by group size (Mentioned in #306). Popular groups sortBy -size
|
@Zren |
Will lazily update over 2h after pushing.
Linkifies Popular Group heading. Depends on #332 for CSS.
Edit:
Need to merge #332 beforehand, but otherwise this is PR READY.
I changed the sort order for popular groups to be by size rather than rating.