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

feat: Add event to highlight active menu #51207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Mar 3, 2025

See nextcloud/tables#1477 (comment)

Summary

Right now some modules uses css manipulations to mark some items as active. Would be nice to have proper API for that.

TODO

  • Decide what we want: event or OC method

Checklist

Signed-off-by: Kostiantyn Miakshyn <[email protected]>
@Koc Koc force-pushed the feature/highlight-active-menu branch from cbe5811 to 15a1b00 Compare March 3, 2025 15:42
@nickvergessen nickvergessen removed their request for review March 3, 2025 15:45
@nickvergessen nickvergessen requested review from a team, nfebe and szaimen and removed request for a team March 3, 2025 15:45
@@ -88,10 +88,12 @@ export default defineComponent({

mounted() {
subscribe('nextcloud:app-menu.refresh', this.setApps)
subscribe('nextcloud:app-menu.active', this.setActive)
Copy link
Contributor Author

@Koc Koc Mar 3, 2025

Choose a reason for hiding this comment

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

IMHO method in OC namespace is better. This allow us to write more universal apps to support multiple NC versions:

if (OC.setNavigationActive) {
  // we are on newer NC api and can use method
} else {
  // we are on legacy NC api and should do css manipulations to preserve BC
}

Copy link
Contributor

Choose a reason for hiding this comment

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

OC. shall not be used by apps, its private API and might change at any time.
Also OCP. is deprecated as we aim to remove all exposed global states and only provide version agnostic API through packages.

@susnux
Copy link
Contributor

susnux commented Mar 3, 2025

Right now some modules uses css manipulations to mark some items as active. Would be nice to have proper API for that.

I do not get the use case here as it is not possible to switch apps without reloading the page?

@@ -88,10 +88,12 @@ export default defineComponent({

mounted() {
subscribe('nextcloud:app-menu.refresh', this.setApps)
subscribe('nextcloud:app-menu.active', this.setActive)
Copy link
Contributor

@susnux susnux Mar 3, 2025

Choose a reason for hiding this comment

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

Suggested change
subscribe('nextcloud:app-menu.active', this.setActive)
subscribe('nextcloud:app-menu:activate', this.setActive)

https://github.com/nextcloud-libraries/nextcloud-event-bus?tab=readme-ov-file#naming-convention

@szaimen szaimen removed their request for review March 3, 2025 17:18
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.

2 participants