Skip to content
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

Added reverse sorting for all listings. #309

Merged
merged 2 commits into from
Aug 21, 2014
Merged

Added reverse sorting for all listings. #309

merged 2 commits into from
Aug 21, 2014

Conversation

jerone
Copy link
Contributor

@jerone jerone commented Aug 21, 2014

Fixes #292

Added reverse sorting to listings:

  • scripts
  • script issues
  • libraries
  • library issues
  • groups
  • group scripts
  • (sub)discussions
  • users
  • user scripts
  • user issues
  • flagged scripts
  • flagged libraries
  • flagged users
  • graveyard

Also fixed ordering Removed By in Removed Items list;

@jerone
Copy link
Contributor Author

jerone commented Aug 21, 2014

@Martii commented on 21 aug. 2014 21:07 CEST:

Bug on

Actually this is not a bug, but should the sorting be removed on that column (Removed By). Same issue as with #306 (comment)

@Martii
Copy link
Member

Martii commented Aug 21, 2014

When removed by shows "Marti" at the top and "Zren" at the bottom and one clicks sort by remover... "Zren" should move up to the top. So yes it is a bug. :)

@jerone
Copy link
Contributor Author

jerone commented Aug 21, 2014

@Martii commented on 21 aug. 2014 21:17 CEST:

When removed by shows "Marti" at the top and "Zren" at the bottom and one clicks sort by remover... "Zren" should move up to the top. So yes it is a bug. :)

Oke, it's a bug. It was a bug before this PR and to my opinion out of the scope of this PR. You are free to make an issue and submit a PR to remove this column ordering if you want.

@Martii
Copy link
Member

Martii commented Aug 21, 2014

Why not fix it now so your PR is not blocked by this? ... You are in this code area.

@Zren
Copy link
Contributor

Zren commented Aug 21, 2014

removerName instead of remover

@jerone
Copy link
Contributor Author

jerone commented Aug 21, 2014

@Zren commented on 21 aug. 2014 21:27 CEST:

removerName instead of remover

YES, that fixed it. Will add it to this PR.

@Martii
Copy link
Member

Martii commented Aug 21, 2014

Btw how do we increment the install count for libs?

@Martii
Copy link
Member

Martii commented Aug 21, 2014

Is http://localhost:8080/?orderBy=updated&orderDir=desc&flagged=true backwards terminology wise? ... in general "Last Update"? n/m

@jerone
Copy link
Contributor Author

jerone commented Aug 21, 2014

Will add it to this PR.

Hmm weird, just pushed this fix to my branch but doesn't show up here...

@Martii
Copy link
Member

Martii commented Aug 21, 2014

doesn't show up here

Have to put a #309 in your commit summary. ...Could be your master is off too. GH is kind of weird in this area.

@jerone
Copy link
Contributor Author

jerone commented Aug 21, 2014

@Martii commented on 21 aug. 2014 21:53 CEST:

doesn't show up here

Have to put a #309 in your commit summary. ...Could be your master is off too. GH is kind of weird in this area.

I think I started this PR wrong.
At the bottom of the PR it's also saying Add more commits by pushing to the issue-292 branch on OpenUserJs/OpenUserJS.org. which should have been jerone/OpenUserJS.org.

@Martii
Copy link
Member

Martii commented Aug 21, 2014

Yah I see that now... you did it directly on OUJS instead of doing it on your fork first.

@jerone
Copy link
Contributor Author

jerone commented Aug 21, 2014

@Martii commented on 21 aug. 2014 22:00 CEST:

Yah I see that now... you did it directly on OUJS instead of doing it on your fork first.

Don't know how, but probably how my tool makes a PR. Won't do that again. Anyway, fix is now added to this PR.

@Martii Martii added the bug label Aug 21, 2014
@Martii
Copy link
Member

Martii commented Aug 21, 2014

Adding bug label so we can merge this eventually. So I'm guessing my question will remain unanswered? It's difficult to test the sort order on libs if there's no install count other than zero. ;)


+1 Tests okay minus the lib install count since they are all zero heh... Nice job... PR READY still? @jerone

@Martii Martii removed the PR READY label Aug 21, 2014
@Martii
Copy link
Member

Martii commented Aug 21, 2014

These are minor but could be useful to fix...

{{{ versus using {{ in the mustache views. Is there a reason why you are unescaping the &xx; values?

Also pushing it right to the line with your var statement not being first in the function here. ;) :)

Unmarking as PR ready since you didn't answer earlier. :P :)

Martii added a commit that referenced this pull request Aug 21, 2014
Added reverse sorting for all listings.

---

Auto merge as per sizzle... will fix mustache in a moment
@Martii Martii merged commit 080f65e into master Aug 21, 2014
Martii pushed a commit to Martii/OpenUserJS.org that referenced this pull request Aug 21, 2014
* STYLEGUIDE fixes
* Named local variable to be local instead of argument
* Use `{{` instead of `{{{` to present any boogs that may appear later on... makes them quite visible.
Martii added a commit that referenced this pull request Aug 21, 2014
@Martii Martii deleted the issue-292 branch August 21, 2014 23:13
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug You've guessed it... this means a bug is reported. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. question A question has been encountered by anyone and has remained unanswered until cleared.
Development

Successfully merging this pull request may close these issues.

Sort Order needed for Script Lists
3 participants