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

fix: Allow to set app order on navigation entries added by closures #41341

Merged
merged 8 commits into from
Nov 14, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Nov 8, 2023

Summary

Some apps, e.g. talk, use the navigation manager to inject the navigation entry instead of using the appinfo.xml
This caused issues as the app property was not set.

This moves some logic from init of the NavigationManager to the add function and refactors the app order config to use navigation IDs rather than app names + index of navigation from appinfo.xml

Also adjusted frontend code for that change.

Checklist

@susnux susnux added bug 3. to review Waiting for reviews labels Nov 8, 2023
@susnux susnux force-pushed the fix/apporder-on-closures branch 2 times, most recently from e9c5ec2 to ed87cd0 Compare November 8, 2023 13:45
@AndyScherzinger AndyScherzinger added this to the Nextcloud 28 milestone Nov 8, 2023
@susnux susnux force-pushed the fix/apporder-on-closures branch 2 times, most recently from 64d17da to b14a7c4 Compare November 8, 2023 21:56
@susnux susnux force-pushed the fix/apporder-on-closures branch from b14a7c4 to 492344e Compare November 9, 2023 02:29
lib/private/NavigationManager.php Outdated Show resolved Hide resolved
tests/lib/App/AppManagerTest.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv removed their request for review November 9, 2023 08:28
@blizzz blizzz mentioned this pull request Nov 9, 2023
@susnux susnux force-pushed the fix/apporder-on-closures branch from 492344e to 9ebd467 Compare November 9, 2023 10:32
@susnux susnux requested a review from come-nc November 9, 2023 10:32
lib/private/App/AppManager.php Outdated Show resolved Hide resolved
lib/private/NavigationManager.php Show resolved Hide resolved
tests/lib/App/AppManagerTest.php Outdated Show resolved Hide resolved
@susnux susnux force-pushed the fix/apporder-on-closures branch from aa45d93 to cada718 Compare November 9, 2023 15:42
@susnux susnux requested a review from come-nc November 9, 2023 15:42
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

The default situation is not clear to me but it’s working as expected. (Also I do not know of any application actually registering several links?)

lib/private/App/AppManager.php Show resolved Hide resolved
@blizzz blizzz mentioned this pull request Nov 10, 2023
@susnux susnux force-pushed the fix/apporder-on-closures branch from cada718 to b3da564 Compare November 10, 2023 19:35
@blizzz
Copy link
Member

blizzz commented Nov 14, 2023

/compile /

@susnux susnux force-pushed the fix/apporder-on-closures branch from b3da564 to 927e7fc Compare November 14, 2023 19:04
@blizzz blizzz merged commit 7791b48 into master Nov 14, 2023
48 of 50 checks passed
@blizzz blizzz deleted the fix/apporder-on-closures branch November 14, 2023 20:45
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.

5 participants