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.

Sign up for GitHub

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

Jump to bottom

Add default client side sort #1972

Merged
merged 2 commits into from
Sep 19, 2019
Merged

Add default client side sort #1972

merged 2 commits into from
Sep 19, 2019

Conversation

PVince81
Copy link
Contributor

Description

To sort by folder, file, then name using natural sort.
The utility functions were imported from OC core.

Related Issue

Ref #1854 (comment)

Motivation and Context

We need at least one default sort to make the list look less ugly when it has a mix of files and folders.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@PVince81 PVince81 self-assigned this Sep 13, 2019
@PVince81 PVince81 mentioned this pull request Sep 13, 2019
Closed
@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIUpload failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5084/

20190913-162402-192.png
20190913-162437-928.png
20190913-162504-804.png
20190913-162531-778.png
20190913-162608-033.png
20190913-162645-409.png
20190913-163044-352.png
20190913-163121-468.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUITrashbin failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5084/

20190913-162801-338.png
20190913-162801-566.png
20190913-162801-828.png
20190913-162802-306.png
20190913-162843-030.png
20190913-162843-246.png
20190913-162843-520.png
20190913-162843-797.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIFavorites failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5084/

20190913-162739-578.png
20190913-162820-724.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5084/

20190913-163147-704.png
20190913-163229-906.png
20190913-163258-964.png
20190913-163327-838.png
20190913-163347-667.png
20190913-163407-414.png
20190913-163533-456.png
20190913-163554-721.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5084/

20190913-162638-629.png
20190913-162710-145.png
20190913-163839-603.png
20190913-163918-591.png
20190913-163954-053.png
20190913-164029-643.png
20190913-164056-929.png
20190913-164125-018.png
20190913-164151-677.png
20190913-164218-264.png
20190913-164245-162.png
20190913-164311-552.png
20190913-164338-009.png
20190913-164404-710.png
20190913-164434-492.png
20190913-164504-298.png

@PVince81
Copy link
Contributor Author

Sorted it out (pun intended).

Sorting by folder/file and then natural sort based on OC 10's algorithm (with a weird chunkify function). I assume we'll want to keep the behavior as before so I imported said function.

Next up: working on acceptance

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUISharingInternalUsers failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5102/

20190916-150414-749.png
20190916-150414-993.png
20190916-150439-048.png
20190916-150439-334.png

@PVince81 PVince81 marked this pull request as ready for review September 16, 2019 17:00
@PVince81
Copy link
Contributor Author

Pushed a fix for the tests.

Please review!

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUILogin failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/5107/

20190916-171902-904.png
20190916-171918-561.png

@LukasHirt
Copy link
Collaborator

@PVince81 We could also just add one more sort after the name sort in getters which would contain the check for folder:

if (fileInfo1.type === 'folder' && fileInfo2.type !== 'folder') {
  return -1
}
if (fileInfo1.type !== 'folder' && fileInfo2.type === 'folder') {
  return 1
}

We'd then lose the more complex function which seems to me too complicated when we can use js sort.

@PVince81
Copy link
Contributor Author

@LukasHirt for performance reasons I think it's better to only call sort once with a single comparator instead of two comparators and two set of arrays. I don't think this function is that complicated to understand. I can add code comments if it can help.

Vincent Petry added 2 commits September 18, 2019 17:30
Added client side sort by name
1787140 
Sorts by file type (folder or file) and then alphabetically using
natural sort.
Add acceptance tests for default natural sort
3879e1d
@PVince81
Copy link
Contributor Author

rebased and solved conflict

@LukasHirt LukasHirt merged commit a3b0d4a into master Sep 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the default-sort branch September 19, 2019 10:18
@PVince81 PVince81 mentioned this pull request Sep 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasHirt LukasHirt LukasHirt approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@individual-it individual-it Awaiting requested review from individual-it

Assignees

@PVince81 PVince81

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PVince81 @ownclouders @LukasHirt