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

Move app menu to vue #33728

Merged
merged 8 commits into from
Aug 31, 2022
Merged

Move app menu to vue #33728

merged 8 commits into from
Aug 31, 2022

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Aug 28, 2022

Move app menu to vue in preparation for further adjustments of #33568

  • Fix current app indicator
  • Properly fix resizing
  • Notification indicator in actions menu
  • Aria-label on more apps button
  • Move to bar active indicator as discussed with @jancborchardt
  • Fix focus frame
Screen.Recording.2022-08-29.at.19.25.07.mov

@juliusknorr juliusknorr requested review from a team, PVince81, Pytal, szaimen, skjnldsv, CarlSchwan and artonge and removed request for a team August 29, 2022 07:20
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 29, 2022
@juliusknorr juliusknorr added this to the Nextcloud 25 milestone Aug 29, 2022
@CarlSchwan
Copy link
Member

The focus frame is now touching the border of the header, causing some contrast issue since there is no clear separation between the focus frame and outside the focus frame.

image

@szaimen
Copy link
Contributor

szaimen commented Aug 29, 2022

@juliushaertl would it be possible to add an API for re-ordering the apps or hiding some of them? I am thinking about the apporder app that will probably break after this PR...

@juliusknorr
Copy link
Member Author

@juliushaertl would it be possible to add an API for re-ordering the apps or hiding some of them? I am thinking about the apporder app that will probably break after this PR...

Possible yes, but not planned yet. There is some long discussion about this in #4917

@juliusknorr
Copy link
Member Author

@CarlSchwan Addressed :)

@PVince81 PVince81 mentioned this pull request Aug 29, 2022
8 tasks
@juliusknorr juliusknorr added the 4. to release Ready to be released and/or waiting for tests to finish label Aug 31, 2022
@PVince81
Copy link
Member

there are JS test failures related to snap/snapper that need to be adjusted
maybe we changed the width?

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@juliusknorr
Copy link
Member Author

Thanks @danxuliu ❤️

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Some details:

  • The active indicator is a bit low down, would be nice if it’s visually inbetween the icon and the content. Just a few px further up :)
  • With a light background selected from the Dashboard, is the same logic of making the header bar icons dark, so they are visible?
  • As per the video, the focus effect is still overlapping the text, or is the vid outdated, as in @CarlSchwan’s comment it also looked different?

@juliusknorr
Copy link
Member Author

With a light background selected from the Dashboard, is the same logic of making the header bar icons dark, so they are visible?

Something we can do once the background image is a global option: #33733

As per the video, the focus effect is still overlapping the text, or is the vid outdated, as in @CarlSchwan’s comment it also looked different?

I didn't touch that yet, to first get the major change in, still tracked as open in #33741

The active indicator is a bit low down, would be nice if it’s visually inbetween the icon and the content. Just a few px further up :)

Will adjust.

@juliusknorr
Copy link
Member Author

Screenshot 2022-08-31 at 14 39 17

@jancborchardt

@@ -36,7 +36,8 @@ import { setUp as setUpMainMenu } from './components/MainMenu'
import { setUp as setUpUserMenu } from './components/UserMenu'
import PasswordConfirmation from './OC/password-confirmation'

const breakpointMobileWidth = getComputedStyle(document.documentElement).getPropertyValue('--breakpoint-mobile')
// keep in sync with core/css/variables.scss
const breakpointMobileWidth = 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about this... yet another place where we need to maintain this number...

Copy link
Member Author

@juliusknorr juliusknorr Aug 31, 2022

Choose a reason for hiding this comment

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

This is breaking the jsunit tests it seems, so I only reverted to the previous state. Something to look into at some later point.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks super nice! :)

@juliusknorr
Copy link
Member Author

Failure unrelated

@juliusknorr juliusknorr merged commit b617f39 into master Aug 31, 2022
@juliusknorr juliusknorr deleted the enh/app-menu-vue branch August 31, 2022 14:17
tcitworld added a commit that referenced this pull request Dec 8, 2022
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
nextcloud-command pushed a commit that referenced this pull request Dec 8, 2022
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
nextcloud-command pushed a commit that referenced this pull request Dec 8, 2022
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
nextcloud-command pushed a commit that referenced this pull request Dec 16, 2022
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
tcitworld added a commit that referenced this pull request Jan 4, 2023
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
tcitworld added a commit that referenced this pull request Jan 4, 2023
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
nickvergessen pushed a commit that referenced this pull request Jan 5, 2023
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
nickvergessen pushed a commit that referenced this pull request Jan 5, 2023
Which was removed in the Vue rewrite in #33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
akhil1508 pushed a commit to e-foundation/server that referenced this pull request Sep 28, 2023
Which was removed in the Vue rewrite in nextcloud#33728. This breaks things like nextcloud/external#79

Signed-off-by: Thomas Citharel <[email protected]>
Signed-off-by: Akhil <[email protected]>
@joshtrichards joshtrichards mentioned this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants