Skip to content

Conversation

@andy-armstrong
Copy link
Contributor

Added sorting to the new pagination logic for STUD-995.

This is a rebased version of my original pull request: https://github.com/edx/edx-platform/pull/2066.

@cahrens @singingwolfboy Could you re-review please.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean the sort direction will default to ascending if not specified? And don't we want a default of descending?

@andy-armstrong
Copy link
Contributor Author

@frrrances Could you give this a quick check too, please. Also, did you see Christina's comment about adding a file when sorting such that the file isn't visible. She suggested always reverting the sort to most recently added when adding a file so that the new file will be at the top. Does that sound reasonable to you?

@cahrens
Copy link

cahrens commented Jan 6, 2014

👍 After clarifying the default value in assets.py and changing the behavior of adding a file (when non-default sort is selected).

@andy-armstrong
Copy link
Contributor Author

@cahrens Thanks for catching those issues. Frances agreed with the idea of changing back to the default sort when adding a new item, so I've implemented that and added unit tests. I've also fixed the default value handling, and submitted a ticket to have 'defaultname' be indexed. I'll wait to squash the commits until the changes are approved.

@cahrens
Copy link

cahrens commented Jan 7, 2014

@frrrances Thanks for the additional styling. Looks good to me.

@andy-armstrong
Copy link
Contributor Author

Here's what I hope to be the final squashed commit. Also, at @frrrances request I've added 'ascending'/'descending' to the name of the sort order.

@cahrens @singingwolfboy Please re-review.

AUTHORS Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to modify the AUTHORS file -- doing so will just result in conflicts for other pull requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an accidental merge conflict (I added myself as an author once with a new line and once without it). I've backed out this change.

@andy-armstrong
Copy link
Contributor Author

@cahrens @singingwolfboy addressed DB's issues.

@singingwolfboy
Copy link
Contributor

👍 once the tests pass.

@frrrances
Copy link
Contributor

@talbs can you review as well since I worked on this feature?

@cahrens
Copy link

cahrens commented Jan 7, 2014

@andy-armstrong Need to rebase.

@andy-armstrong
Copy link
Contributor Author

@cahrens Rebased again :-)

@cahrens
Copy link

cahrens commented Jan 8, 2014

👍

Added sorting to the new pagination logic for STUD-995.
@talbs
Copy link
Contributor

talbs commented Jan 8, 2014

Everything looks good from a UX and Front End Perspective. Nice work!

👍

andy-armstrong added a commit that referenced this pull request Jan 8, 2014
Add sorting to Studio's Files & Uploads page
@andy-armstrong andy-armstrong merged commit eeb5f81 into master Jan 8, 2014
@andy-armstrong andy-armstrong deleted the andya/asset-sorting branch January 8, 2014 17:10
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.

6 participants