Kh/dev/add simplenavigation v33 - #24
Conversation
There was a problem hiding this comment.
Pull request overview
Updates SimpleNavigation’s header integration for Nextcloud v33 (new .header-start/.header-end layout and unified search mount) and refines the Vue user menu rendering, icon strategy, and localization.
Changes:
- Adapt header DOM hooks/styles for NC v33 (hide default header-start early; update unified-search wrapper styling; ensure header-end alignment).
- Mount the UserMenu only when authenticated and gate the webmail action on both
webmailUrlandhasEmailProduct. - Switch menu icons to
@mdi/js+NcIconSvgWrapperand add initial l10n strings for multiple locales.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/styles.scss | Updates header/unified-search styling for NC v33 and header-end alignment/opacity overrides. |
| src/main.ts | Updates header selectors to .header-start/.header-end and mounts UserMenu only for authenticated users. |
| src/components/UserMenu.vue | Switches to MDI paths + NcIconSvgWrapper, gates webmail link, and adjusts layout CSS. |
| package.json | Adds @mdi/js and adjusts dependencies. |
| package-lock.json | Locks @mdi/js and updates dependency ranges. |
| lib/Listener/BeforeTemplateRenderedListener.php | Provides isLoggedIn initial state and scopes other initial-state values to logged-in users. |
| l10n/sv.json | Adds Swedish translations for new menu strings. |
| l10n/sv.js | Registers Swedish translations for new menu strings. |
| l10n/nl.json | Adds Dutch translations for new menu strings. |
| l10n/nl.js | Registers Dutch translations for new menu strings. |
| l10n/it.json | Adds Italian translations for new menu strings. |
| l10n/it.js | Registers Italian translations for new menu strings. |
| l10n/fr.json | Adds French translations for new menu strings. |
| l10n/fr.js | Registers French translations for new menu strings. |
| l10n/es.json | Adds Spanish translations for new menu strings. |
| l10n/es.js | Registers Spanish translations for new menu strings. |
| l10n/en.json | Adds English strings for new menu labels. |
| l10n/en.js | Registers English strings for new menu labels. |
| l10n/de_DE.json | Adds German (de_DE) translations for new menu strings. |
| l10n/de_DE.js | Registers German (de_DE) translations for new menu strings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // In NC v33 the unified search mounts as <div class="unified-search-menu"> (the | ||
| // Vue component root that replaces the old #unified-search placeholder div). | ||
| // Inject colour variables and prevent the wrapper from growing wider than its button. | ||
| .unified-search-menu { | ||
| --color-background-plain-text: var(--ion-text); |
8399fd0 to
334d89d
Compare
Move login state check earlier and conditionally render user menu and related initial state only for authenticated users.
334d89d to
b8245bd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/styles.scss:6
- The selector used to hide Nextcloud’s header-start is unscoped (".header-start"), so it will hide any element with that class anywhere in the page. Since the intent is specifically to hide the header’s logo/appmenu area, scope this rule to #header to reduce unintended side effects and match the surrounding comment.
.header-start {
display: none !important;
}
src/main.ts:24
- This comment still mentions a #unified-search element, but the stylesheet notes that in NC v33 unified search mounts as a ".unified-search-menu" root. Updating the comment avoids misleading future maintenance/debugging.
// └── .header-end ← original NC div, kept intact
// ├── #unified-search ← NC unified search already mounted here
// ├── #simplenavigation-usermenu ← our UserMenu Vue 3 app (inserted)
src/components/UserMenu.vue:46
- The Webmail link is icon-only, so relying on a title attribute alone is not sufficient for accessible names in all assistive technologies. Add an aria-label (you can reuse the same localized string).
:href="props.webmailUrl"
target="_blank"
rel="noopener noreferrer"
class="ion-header-action"
:title="t('simplenavigation', 'IONOS Webmail')">
tests/unit/Listener/BeforeTemplateRenderedListenerTest.php:106
- The listener now provides public state for anonymous rendering so the logo can render, which implies the JS/CSS assets must also be registered for anonymous requests. This test verifies state keys, but doesn’t assert scripts/styles are enqueued for the anonymous path, so a regression could reintroduce the early return without failing tests.
$this->assertFalse($this->providedState['isLoggedIn']);
$this->assertSame('/index.php', $this->providedState['homeUrl']);
foreach (['displayName', 'logoutUrl', 'settingsUrl', 'webmailUrl', 'hasEmailProduct', 'securityUrl', 'helpUrl'] as $key) {
$this->assertArrayNotHasKey($key, $this->providedState);
}
printminion-co
left a comment
There was a problem hiding this comment.
review done (approved) with fixups.
Signed-off-by: Kai Henseler <kai.henseler@strato.de>
Adds translation files in both .js and .json formats for English, German, Spanish, French, Italian, Dutch, and Swedish languages. Ported from legacy theming header Signed-off-by: Kai Henseler <kai.henseler@strato.de>
b8245bd to
2d3cde9
Compare
No description provided.