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

Frapell sort folder contents #850

Merged
merged 13 commits into from
Jul 4, 2018
Merged

Frapell sort folder contents #850

merged 13 commits into from
Jul 4, 2018

Conversation

frapell
Copy link
Member

@frapell frapell commented May 30, 2018

I included the https://datatables.net/ library as a new pattern pat-datatables, and included it to be used in the structure, so columns are sortable. I also included it with the plone bundle, so this is also available to editors when manually creating a table using TinyMCE (Maybe we could add in the future, a style in the drop down for TinyMCE to make it more easy to use... out of the scope of this PR though)

In addition, I went ahead and solved a couple of issues that the structure pattern was having (clicking disabled buttons, and allowing reordering after filtering with a query), as well as adding a new "250" option to the pagination size to show more items.

This is coming from gh-848

@frapell frapell requested review from thet and vangheem May 30, 2018 17:13
@frapell
Copy link
Member Author

frapell commented May 30, 2018

@thet @vangheem @jensens Once this PR is merged, we need to also merge plone/Products.CMFPlone#2436 and plone/plone.app.upgrade#163

@jensens
Copy link
Member

jensens commented Jun 1, 2018

LGTM, I would like to have @thet also had a look at it. From what I see all targets are at Plone master, so Plone 5.2 - are you planning a backport to 5.1 as well?

@frapell
Copy link
Member Author

frapell commented Jun 1, 2018

Totally. Once I know the functionality is accepted I plan to backport to 5.1 and 5.0 as well

@frapell
Copy link
Member Author

frapell commented Jun 6, 2018

@thet When you have a chance, please don't forget to review this PR, please :)

Copy link
Member

@thet thet left a comment

Choose a reason for hiding this comment

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

#cannotdeletethisobsoletemessagethroughgithubsui #dontknowwhy

Copy link
Member

@thet thet left a comment

Choose a reason for hiding this comment

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

All in all looks good to me, except these minor points:

  • There is no explicit option to reset the datatables ordering, so that one is able to manually reorder items again.
  • When clicking in the paginator (e.g. 250) the order is reset but the " Can not order items while manually sorting a column" is still in place. (items can be reordered, though).
  • When sorting via datatables, the "Rearrange" button is still active and works. Which is actually fine - no reason to deactivate (except the reorder-warning-message makes one believe that it won't work)
  • Why didn't the "All" paginagtion button work?

@thet thet force-pushed the frapell-sort-folder_contents branch from a6613bb to f01a875 Compare June 7, 2018 16:01
@frapell
Copy link
Member Author

frapell commented Jun 8, 2018

Thanks for the feedback, will work on them and notify you when done

@thet thet force-pushed the frapell-sort-folder_contents branch from f01a875 to e51f759 Compare June 14, 2018 12:02
@thet
Copy link
Member

thet commented Jun 14, 2018

@frapell I did a rebase - I hope it didn't go wrong.
Tests are failing. I can't remember if they passed before my rebase... do you, @frapell ?

@frapell
Copy link
Member Author

frapell commented Jun 14, 2018

@thet Yeah, they were, I will try to fix them when I can have some time to work on your suggestions... I wasn't able these past days :(

@thet thet force-pushed the frapell-sort-folder_contents branch from e51f759 to a6613bb Compare June 14, 2018 14:31
@thet
Copy link
Member

thet commented Jun 14, 2018

@frapell - restored old pre-rebased version. Let's see if tests are green.

@frapell
Copy link
Member Author

frapell commented Jul 3, 2018

@thet I merged master back into the branch and added the ability to reset the ordering back to default without the need to refresh the whole view.

As per your review points:

  • There is no explicit option to reset the datatables ordering, so that one is able to manually reorder items again.

    • This is now fixed. I added the ability to include a button in the status message, and it shows up like this:
      image
      This is the cleanest way I found to allow "unsorting"
  • When clicking in the paginator (e.g. 250) the order is reset but the " Can not order items while manually sorting a column" is still in place. (items can be reordered, though).

    • Fixed, now the status message will be removed when changing the view
  • When sorting via datatables, the "Rearrange" button is still active and works. Which is actually fine - no reason to deactivate (except the reorder-warning-message makes one believe that it won't work)

    • Well, the "rearrange" would change all items in the folder, so, as you say, it shouldn't be disabled, however I changed the status message to read "Can not drag and drop items to reorder while manually sorting a column" so it would be more explicit
  • Why didn't the "All" paginagtion button work?

    • Currently, the batching is done using "backbone" and I couldn't find a way of choosing "All" as option for it. So, my workaround was to provide a really big number while showing "All" as text. However, that didn't work, because there is a hard coded limit in https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/vocabulary.py#L36, which throws and exception "Exception: Max batch size is 500". So, to allow "All" as an option, it would require major refactoring, and I think it would require a new PR, out of the scope of this one, so will just provide an additional option for 250 items, which I think is reasonable, and maybe add an "All" option in the future.

@frapell
Copy link
Member Author

frapell commented Jul 3, 2018

@thet All green :)

@frapell
Copy link
Member Author

frapell commented Jul 4, 2018

@thet Do you want to review again? or with your approval, I can just go ahead and merge?

@thet thet merged commit b91e390 into master Jul 4, 2018
@thet thet deleted the frapell-sort-folder_contents branch July 4, 2018 15:32
@thet
Copy link
Member

thet commented Jul 4, 2018

Merging done for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants