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 sorting BREAD as tree view if BREAD have column - menu_parent #4671

Closed
wants to merge 1 commit into from

Conversation

dzianisyaukhuta
Copy link

@dzianisyaukhuta dzianisyaukhuta commented Jan 3, 2020

I did a great job so that we can sort our BREAD resources in a tree view.

If BREAD have column named as menu_parent and isset Order column - you get functional for reorder with tree view.

sortt

VoyagerBaseController

VoyagerBaseController order() updated:

  1. A new code in which, depending on the presence of a column menu_parent, new functions are called for constructing templates.
  2. Removed the transfer of parameters - display_column, dataRow to the view, because the view template has been changed and now does not require these parameters.

VoyagerBaseController - order_results_tree_array() added:
New recursive function that builds a tree array from resources, which is called in the order()

VoyagerBaseController - order_results_tree_build() added:
New recursive function that builds a view output from view templates, which is called in the order()

VoyagerBaseController - update_order() updated:

  1. Fixed a bug. Previously, if descending ordering was assigned, the resources were still updated as ascending, and after refreshing the page we saw this bug.
    Therefore, I added a new function reverseArray() which reverses the multidimensional array and saves it later as needed.
  2. Created a condition and now if the table/model has a 'menu_parent' column then it calls the new recursive function update_order_tree()

VoyagerBaseController - reverseArray() added:
New function. Used in update_order() for reverse multidimensional array.

VoyagerBaseController - update_order_tree() added:
New Recursive function called in update_order() to update tree resources

View Templates

All new and updated view templates called and used as renders in controller.

bread/order.blade.php updated

  1. Buttons added Collapse all, Expand all if results_tree_view from controller is true.
  2. Remove foreach $results and added conditions for $results
  3. Scripts updated. Added orderDirection as param in ajax request.

bread/order-tree-wrapper-without-childs.blade.php added
New view template as Wrapper for results item without childrens (not tree view).

bread/order-tree-wrapper-with-childs.blade.php added
New view template as Wrapper for results item with childrens (tree view).

order-tree-item.blade.php added
New view template as item of results.

Attention to enable functionality in BREAD reorder:

  • Column named menu_parent needs in your BREAD
  • Order column must be assigned and not be empty

Dzianis Yaukhuta
dzianisyaukhuta@gmail.com

@dzianisyaukhuta dzianisyaukhuta mentioned this pull request Jan 3, 2020
Copy link
Collaborator

@fletch3555 fletch3555 left a comment

Choose a reason for hiding this comment

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

Handful of code style changes.

  1. You've moved all of the presentation logic into the controller. We have historically maintained it at the view tier, including the logic used to include other views. I'd like to hear opinions from @emptynick or @MrCrayon, but I'm thinking I'd like to see that pattern maintained.
  2. When committing to open source projects (or really anything you didnt create yourself), please follow existing code styling. Specifically, methods are camelCase, not snake_case, among others(indentation, brace/bracket placement, etc... though StyleCI should catch and fix most of that stuff).
  3. Large collections of functionality/methods like this are perfect candidates for traits. Resolving Creating Accounts  #1 will likely reduce the number of methods and make this less relevant.
  4. Tests. We need some kind of test submitted with this as proof that it actually works. We don't expect 100% test coverage(though it'd be nice), but we simply can't accept it with nothing.

Thanks!

@dzianisyaukhuta
Copy link
Author

Handful of code style changes.

  1. You've moved all of the presentation logic into the controller. We have historically maintained it at the view tier, including the logic used to include other views. I'd like to hear opinions from @emptynick or @MrCrayon, but I'm thinking I'd like to see that pattern maintained.
  2. When committing to open source projects (or really anything you didnt create yourself), please follow existing code styling. Specifically, methods are camelCase, not snake_case, among others(indentation, brace/bracket placement, etc... though StyleCI should catch and fix most of that stuff).
  3. Large collections of functionality/methods like this are perfect candidates for traits. Resolving Creating Accounts  #1 will likely reduce the number of methods and make this less relevant.
  4. Tests. We need some kind of test submitted with this as proof that it actually works. We don't expect 100% test coverage(though it'd be nice), but we simply can't accept it with nothing.

Thanks!

About 1.
What is wrong with point 1 ? All logic is processed in the controller. A wrapper with children takes one template, a wrapper without children takes another template. Both wrappers take a single element template. And there is a super global template that displays this all.

@MrCrayon
Copy link
Collaborator

MrCrayon commented Jan 4, 2020

  1. You've moved all of the presentation logic into the controller. We have historically maintained it at the view tier, including the logic used to include other views. I'd like to hear opinions from @emptynick or @MrCrayon, but I'm thinking I'd like to see that pattern maintained.

About 1.
What is wrong with point 1 ? All logic is processed in the controller. A wrapper with children takes one template, a wrapper without children takes another template. Both wrappers take a single element template. And there is a super global template that displays this all.

IMHO that's presentation logic and in MVC that should stay in the view.

Adding to what @fletch3555 said, at first glance, I'd like to add:

  • Tests are failing, traling commas in function calls are not allowed in PHP 7.2.
  • I would also like the parent_id field to not be hardcoded as menu_parent.
    To follow Voyager way that should be set with a select in BREAD configuration page but personally I would be ok if we use a Trait with a method that provides parent_id field name and then we check if Trait is used.

I'll have a better look at this PR as soon as I can.

If we can figure it out all details it could be a nice feature, thanks.

P.S.
Adding:
Closes #4641
will automatically close that issue once this PR is merged (use Fixes in case of bugs)

@MrCrayon
Copy link
Collaborator

MrCrayon commented Jan 4, 2020

Before you make further changes you should rebase your branch because:
This branch is 1 commit ahead, 7 commits behind the-control-group:1.3.

You should always be aware of what changes are in your PR, if there is something not related check what's happening like here:

-            if ($model && in_array(SoftDeletes::class, class_uses_recursive($model))) {
+            if ($model && in_array(SoftDeletes::class, class_uses($model))) {

@MrCrayon
Copy link
Collaborator

MrCrayon commented Jan 5, 2020

@dzianisyaukhuta please check #4675 I put you as Co-Author.

I understand you put a lot of effort in this PR and we surely appreciate this and hope you'll keep contributing but there was a bit too much to change.

You can see leaving the presentation logic in the view we only needed to make the listing part a partial that can be recursively included when there are children.
Also your fix to reverse the order would only work on the first level because the ajax call passes an array of objects.

If you have time we still need to implement tests and documentation and I left out the buttons that could also be implemented.
You can clone my branch and open PR in my fork so we can merge it in #4675

Fill free to contact me in Voyager slack group.

@dzianisyaukhuta
Copy link
Author

dzianisyaukhuta commented Jan 10, 2020

Guys. I am not programmer. I am designer with some skills in programming. Can you help me? Or just complete my work? @MrCrayon , can you end my work? Or another. We all need that reorder. I think my knowledge is not enough to complete this PR :(

@MrCrayon
Copy link
Collaborator

@dzianisyaukhuta #4675 already works, but it can't be merged without tests.
I was asking for collaboration because I don't know when I'll be able to add tests there and also I wanted to include you in the works since you were kind enough to submit this PR.

I can close this one and we can move the discussion in the other PR.

@MrCrayon MrCrayon closed this Jan 10, 2020
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.

3 participants