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

[5.0] order site/admin menus in admin ui #38149

Merged
merged 16 commits into from Jul 17, 2023
Merged

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Jun 26, 2022

The site and admin menus displayed in the admin interface sidebar are hard coded to be displayed by id desc

This PR allows the order to be changed using drag and drop from com_menus&view=menus

Pull Request for Issue #38143

When thinking about documentation for this PR I concluded that this is NOT a new feature but its more of a bug that you couldnt sort them before just as you can sort everything else.

To test

Clean install from the prebuilt package in this PR
Create multiple site and admin menus (they dont need any menu items in them for the prupose of this test)
Drag and drop to change the order
Refresh the page and you will see the changes take effect in the sidebar

Update an existing test site using the prebuilt package in this pr
Repeat the steps above.

menus

thanks @HLeithner

currently they are hard coded to be displayed by id desc

this pr will (when finished) allows the order to be set using drag and drop from com_menus&view=menus

This is for the display of the menus in the sidebar and the menus dashboard

todo
- [ ] sql update
- [ ] sql installation
- [ ] saveorderingajax
- [ ] add ordering to menu creation
@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.0-dev.

@HLeithner
Copy link
Member

@brianteeman would be great if you had time to finish this pr I would be happy to merge it into 5.0

@brianteeman
Copy link
Contributor Author

If I knew how I would. That's why I asked for help

@HLeithner HLeithner changed the title [wip] order site menus in admin ui [5.0] order site menus in admin ui Jul 10, 2023
@HLeithner
Copy link
Member

oh sorry I didn't saw your comment, what exactly do you need? or is it the todo list you added where you need help?

@brianteeman brianteeman marked this pull request as ready for review July 12, 2023 07:58
@brianteeman brianteeman marked this pull request as draft July 12, 2023 07:58
installation/sql/mysql/base.sql Outdated Show resolved Hide resolved
installation/sql/postgresql/base.sql Outdated Show resolved Hide resolved
brianteeman and others added 2 commits July 12, 2023 10:03
Co-authored-by: Harald Leithner <leithner@itronic.at>
Co-authored-by: Harald Leithner <leithner@itronic.at>
@brianteeman
Copy link
Contributor Author

@HLeithner sorry - had to look at the code again to see where I had left it.

currently stuck at saveorderingajax

probably missing something obvious but I just cant see it

@HLeithner
Copy link
Member

I updated the controller now it's working

@brianteeman
Copy link
Contributor Author

shucks - i changed the controller but not the model.

will test it tomorrow and see what else needs doing

thanks

@HLeithner
Copy link
Member

Maybe we should update the ordering column of the existing menu entries when adding the ordering column. Something like update #__menus set ordering=id

@brianteeman
Copy link
Contributor Author

yes I was planning on doing that and also checking the sample data plugins

@brianteeman brianteeman changed the title [5.0] order site menus in admin ui [5.0] order site/admin menus in admin ui Jul 13, 2023
@brianteeman brianteeman marked this pull request as ready for review July 13, 2023 09:52
@chmst
Copy link
Contributor

chmst commented Jul 13, 2023

Resolves #39236

@chmst
Copy link
Contributor

chmst commented Jul 13, 2023

The ordering of menu Items will be a challenge. First tests look very promising. Thanks for that @brianteeman

@brianteeman
Copy link
Contributor Author

The ordering of menu Items will be a challenge.

Not sure what you mean. This PR is about ordering the menus not the items

@chmst
Copy link
Contributor

chmst commented Jul 13, 2023

Sorry, forget it. Multi-tasking-failure ;)

@bayareajenn
Copy link

I have tested this item ✅ successfully on 9b311a2

Tested new install and tested by updating an instance. Really great, nice to have. Thanks, Brian. :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38149.

@HLeithner HLeithner merged commit e8d462f into joomla:5.0-dev Jul 17, 2023
3 of 4 checks passed
@HLeithner
Copy link
Member

thanks

1 similar comment
@brianteeman
Copy link
Contributor Author

thanks

@brianteeman brianteeman deleted the menu_order branch July 17, 2023 12:07
@@ -0,0 +1,2 @@
ALTER TABLE "#__menu_types" ADD COLUMN "ordering" int NOT NULL DEFAULT 0 AFTER "client_id";
Copy link
Member

Choose a reason for hiding this comment

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

PostgreSQL doesn't support "AFTER", so this will result in an SQL syntax error when updating on PostgreSQL.

Copy link
Member

Choose a reason for hiding this comment

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

thanks can you fix it please?

Copy link
Member

Choose a reason for hiding this comment

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

Can take a while because I have to boot my environment and update my branches.

Copy link
Member

Choose a reason for hiding this comment

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

PR is #41177 .

Copy link
Member

Choose a reason for hiding this comment

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

I just see the update SQL scripts are also missing the ´/** CAN FAIL **/` installer hint. PR is in preparation.

Copy link
Member

Choose a reason for hiding this comment

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

PR is #41184 .

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.

None yet

10 participants