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.

Sign up for GitHub

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

Jump to bottom

Port user menu to Vue #36232

Merged
merged 1 commit into from
Feb 11, 2023
Merged

Port user menu to Vue #36232

merged 1 commit into from
Feb 11, 2023

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented Jan 19, 2023 •
edited
Loading

Summary

Move PHP user menu markup to Vue wrapped by the focus trapped NcHeaderMenu component

Before After
image image

Requires

Related

Checklist

@Pytal Pytal added the 2. developing Work in progress label Jan 19, 2023
@Pytal Pytal self-assigned this Jan 19, 2023
@Pytal Pytal mentioned this pull request Jan 19, 2023
Closed
2 tasks
@Pytal Pytal force-pushed the enh/a11y-user-menu branch 5 times, most recently from 4c21322 to c2c704a Compare January 25, 2023 02:12
@Pytal Pytal changed the title [WIP] Port user menu Port user menu to Vue Jan 25, 2023
@Pytal
Copy link
Member Author

Pytal commented Jan 25, 2023

/backport to stable25

@Pytal Pytal marked this pull request as ready for review January 25, 2023 02:20
@Pytal Pytal added enhancement 3. to review Waiting for reviews accessibility and removed 2. developing Work in progress labels Jan 25, 2023
@Pytal Pytal added this to the Nextcloud 26 milestone Jan 25, 2023
nickvergessen
nickvergessen reviewed Jan 25, 2023
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Not breaking the external sites app, if that is why my review was requested.
From the technical changes in this PR I'm the wrong person to judge

juliushaertl
juliushaertl reviewed Jan 25, 2023
View reviewed changes
apps/user_status/src/menu.js Outdated
displayName: avatarDiv.dataset.displayname,
disableMenu: true,
disableTooltip: true,
const mountMenu = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we be fully sure that the UserMenu has already been rendered at this point? Otherwise this might need some way of handling to not end up with a menu without the profile link which might become hard to debug later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be as this is only executed when UserMenu has been mounted and emits core:user-menu:mounted

This only works as long as apps/user_status/src/menu.js is loaded before core/src/components/UserMenu.js, but if UserMenu.js loads and emits core:user-menu:mounted before menu.js can subscribe to the event then it wouldn't

For this I adjusted the logic to handle both cases

juliushaertl
juliushaertl reviewed Jan 25, 2023
View reviewed changes
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Just some smaller comments, but looks good otherwise 👍

@Pytal Pytal force-pushed the enh/a11y-user-menu branch 2 times, most recently from a2726b4 to 77771f5 Compare January 26, 2023 02:17
skjnldsv
skjnldsv requested changes Jan 26, 2023
View reviewed changes
apps/user_status/src/menu.js Outdated Show resolved Hide resolved
core/src/views/UserMenu.vue Outdated Show resolved Hide resolved
@Pytal Pytal changed the base branch from enh/allow-empty-route-xml to enh/allow-no-route February 3, 2023 01:25
@skjnldsv skjnldsv added 2. developing Work in progress 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 2. developing Work in progress labels Feb 3, 2023
Base automatically changed from enh/allow-no-route to master February 3, 2023 11:54
@Pytal
Copy link
Member Author

Pytal commented Feb 4, 2023

Rebased

@nickvergessen nickvergessen removed their request for review February 6, 2023 09:50
@miaulalala miaulalala removed their request for review February 7, 2023 13:00
szaimen
szaimen approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Tested together with nextcloud/firstrunwizard#816 and works now but didnt review the code.

(Looks like the internal server issue was somehow caused by the viewer app not being installed).

@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 9, 2023
@Pytal
Port user menu to Vue
c779982 
Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal merged commit b59b8fd into master Feb 11, 2023
@Pytal Pytal deleted the enh/a11y-user-menu branch February 11, 2023 03:22
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 11, 2023
Merged
1 task
@blizzz blizzz mentioned this pull request Feb 13, 2023
Merged
@joshtrichards joshtrichards mentioned this pull request Feb 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@artonge artonge artonge approved these changes

@szaimen szaimen szaimen approved these changes

@JuliaKirschenheuter JuliaKirschenheuter Awaiting requested review from JuliaKirschenheuter

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@tcitworld tcitworld Awaiting requested review from tcitworld

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

Assignees

@Pytal Pytal

Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility enhancement
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
7 participants
@Pytal @szaimen @nickvergessen @juliushaertl @artonge @skjnldsv @nimishavijay