-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
App menu: Fix default order and related issues #62963
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -54,19 +54,19 @@ | |||||||
| :aria-expanded="opened ? 'true' : 'false'" | ||||||||
| @click="onTriggerClick('currentApp')"> | ||||||||
| <template #icon> | ||||||||
| <!-- Settings sub-sections share one generic cog. An inline MDI icon | ||||||||
| inherits the button's currentColor (--color-background-plain-text), | ||||||||
| <!-- Sections of the settings app share one generic cog. An inline MDI | ||||||||
| icon inherits the button's currentColor (--color-background-plain-text), | ||||||||
| so it stays legible on both bright and dark headers without a filter. --> | ||||||||
| <IconCog | ||||||||
| v-if="currentApp.type === 'settings'" | ||||||||
| v-if="isSettingsSection" | ||||||||
| class="app-menu__current-app-cog" | ||||||||
| :size="20" /> | ||||||||
| <img | ||||||||
| v-else | ||||||||
| class="app-menu__current-app-icon" | ||||||||
| :src="currentApp.icon" | ||||||||
| alt="" | ||||||||
| aria-hidden="true"> | ||||||||
| <!-- Outer element carries the header fade, inner one the icon shape. --> | ||||||||
| <span v-else class="app-menu__current-app-icon"> | ||||||||
| <span | ||||||||
| class="app-menu__current-app-glyph" | ||||||||
| :style="currentAppIconStyle" /> | ||||||||
| </span> | ||||||||
| </template> | ||||||||
| <span class="app-menu__current-app-name"> | ||||||||
| {{ displayName }} | ||||||||
|
|
@@ -94,6 +94,14 @@ import logger from '../logger.js' | |||||||
| // Settings IDs that represent actions, not navigable pages. | ||||||||
| const SETTINGS_ACTION_IDS = new Set(['logout']) | ||||||||
|
|
||||||||
| // Sections of the settings app itself. Their names ("Personal settings", | ||||||||
| // "Appearance and accessibility", ...) are too long and varied for the header, | ||||||||
| // so they all show as "Settings". Other settings entries keep their own name. | ||||||||
|
Comment on lines
+97
to
+99
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| const SETTINGS_SECTION_IDS = new Set(['settings_personal', 'settings_administration', 'accessibility_settings']) | ||||||||
|
|
||||||||
| // Entry of the app management page, the target of the "More apps" tile. | ||||||||
| const APP_MANAGEMENT_ID = 'appstore' | ||||||||
|
|
||||||||
| export default defineComponent({ | ||||||||
| name: 'AppMenu', | ||||||||
|
|
||||||||
|
|
@@ -169,18 +177,31 @@ export default defineComponent({ | |||||||
| ?? Object.values(this.settingsList).find((entry) => entry.active && !SETTINGS_ACTION_IDS.has(entry.id)) | ||||||||
| }, | ||||||||
|
|
||||||||
| // Trigger label. Settings sub-section names ("Personal info", | ||||||||
| // "Appearance and accessibility", ...) are too long and varied to | ||||||||
| // surface in the header; collapse them all to a single "Settings". | ||||||||
| isSettingsSection(): boolean { | ||||||||
| return this.currentApp !== undefined && SETTINGS_SECTION_IDS.has(this.currentApp.id) | ||||||||
| }, | ||||||||
|
|
||||||||
| // Trigger label. Sections of the settings app show as "Settings", | ||||||||
| // see SETTINGS_SECTION_IDS. All other entries use their own name, | ||||||||
| // so the app management page shows "Apps" like in the account menu. | ||||||||
|
Comment on lines
+184
to
+186
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| displayName(): string { | ||||||||
| if (!this.currentApp) { | ||||||||
| return '' | ||||||||
| } | ||||||||
| return this.currentApp.type === 'settings' | ||||||||
| return this.isSettingsSection | ||||||||
| ? t('core', 'Settings') | ||||||||
| : this.currentApp.name | ||||||||
| }, | ||||||||
|
|
||||||||
| // The icon is painted through a mask, so entries with a dark icon | ||||||||
| // (the app management page ships one for the settings list) are legible | ||||||||
| // on the header as well. Escaped so a crafted path cannot break out of | ||||||||
| // the url() token, same as AppIcon.vue. | ||||||||
| currentAppIconStyle(): Record<string, string> { | ||||||||
| const icon = this.currentApp?.icon ?? '' | ||||||||
| return { '--app-icon-url': `url("${icon.replace(/["\\]/g, '\\$&')}")` } | ||||||||
| }, | ||||||||
|
|
||||||||
| // aria-label overrides the inner span text, so the displayed name | ||||||||
| // has to be duplicated here for screen readers. | ||||||||
| currentAppLabel(): string { | ||||||||
|
|
@@ -193,7 +214,11 @@ export default defineComponent({ | |||||||
| // utility tile is "More apps" (local app management) for admins and | ||||||||
| // "App store" (apps.nextcloud.com) for everyone else. | ||||||||
| gridItems(): INavigationEntry[] { | ||||||||
| const tail = this.isAdmin ? this.moreAppsEntry : this.appStoreEntry | ||||||||
| // On the app management page the "More apps" tile is the current | ||||||||
| // entry, so it is marked active like any other app tile. | ||||||||
|
Comment on lines
+217
to
+218
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| const tail = this.isAdmin | ||||||||
| ? { ...this.moreAppsEntry, active: this.currentApp?.id === APP_MANAGEMENT_ID } | ||||||||
| : this.appStoreEntry | ||||||||
| return [...this.appList, tail] | ||||||||
| }, | ||||||||
| }, | ||||||||
|
|
@@ -459,13 +484,29 @@ export default defineComponent({ | |||||||
| } | ||||||||
|
|
||||||||
| &__current-app-icon { | ||||||||
| display: flex; | ||||||||
| width: calc(var(--default-grid-baseline) * 5); | ||||||||
| height: calc(var(--default-grid-baseline) * 5); | ||||||||
| // Theme-aware inversion + vertical alpha fade via --header-menu-icon-mask. | ||||||||
| filter: var(--background-image-invert-if-bright); | ||||||||
| // Vertical alpha fade, like the cog and the other header icons. | ||||||||
| mask: var(--header-menu-icon-mask); | ||||||||
| } | ||||||||
|
|
||||||||
| &__current-app-glyph { | ||||||||
| width: 100%; | ||||||||
| height: 100%; | ||||||||
| // Masked rather than shown: app icons ship a hardcoded fill, so the | ||||||||
| // color has to come from the background. Matches AppIcon.vue. | ||||||||
| background-color: var(--color-background-plain-text); | ||||||||
| mask: var(--app-icon-url) center / contain no-repeat; | ||||||||
| } | ||||||||
|
|
||||||||
| // Masked backgrounds are not force-adjusted the way <img> is. | ||||||||
| @media (forced-colors: active) { | ||||||||
| &__current-app-glyph { | ||||||||
| background-color: CanvasText; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| &__current-app-cog { | ||||||||
| mask: var(--header-menu-icon-mask); | ||||||||
| } | ||||||||
|
|
||||||||
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI comments like this are pretty bad. They will get outdated - because no one updates such comments - and once someone / something tries to understand whats going on it will just confuse.
Basically the reason why comments explaining the implementation are an anti-pattern, instead implementation should be self explaining.
(this is just an example there are more below - also using
//will make them pretty useless as they are not even doc blocks).