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

Use NcActions component for NcAvatar #4017

Merged
merged 1 commit into from May 10, 2023
Merged

Use NcActions component for NcAvatar #4017

merged 1 commit into from May 10, 2023

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Apr 28, 2023 •
edited

This PR migrates the NcAvatar component from using the deprecated NcPopoverMenu to NcActions.

Before After
Screenshot 2023-04-29 at 13-06-41 Tasks - Nextcloud Screenshot 2023-04-29 at 13-09-07 Tasks - Nextcloud

Closes #1683, closes #1762

@raimund-schluessler raimund-schluessler mentioned this pull request Apr 28, 2023
Closed
5 tasks
@raimund-schluessler raimund-schluessler added this to the 7.11.0 milestone Apr 28, 2023
@raimund-schluessler raimund-schluessler added 2. developing Work in progress feature: actions Related to the actions components feature: avatar Related to the avatar component technical debt feature: popover Related to the popovermenu component labels Apr 28, 2023
@raimund-schluessler raimund-schluessler marked this pull request as ready for review April 29, 2023 11:15
@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews design Design, UX, interface and interaction design and removed 2. developing Work in progress labels Apr 29, 2023
ShGKme
ShGKme requested changes May 1, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

The loading indicator is visible only on hover

NcAvatar

@artonge artonge modified the milestones: 7.11.0, 7.12.0 May 3, 2023
@raimund-schluessler raimund-schluessler modified the milestones: 7.12.0, 8.0.0 May 5, 2023
@raimund-schluessler raimund-schluessler changed the title Use the NcActions component for NcAvatar Use NcActions component for NcAvatar May 5, 2023
@raimund-schluessler
Use the NcActions component for NcAvatar
6be00c3 
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

The loading indicator is visible only on hover

NcAvatar NcAvatar

Fixed.

@skjnldsv
Copy link
Contributor

skjnldsv commented May 9, 2023 •
edited

NcAvatar

I see something odd on the gif too.
On hover, the three dot should probably not be seen over the avatar, no?
The contrast is quite problematic. It should be full background colour with the three dot svg on top of it?

image

@raimund-schluessler
Copy link
Contributor Author

NcAvatar
I see something odd on the gif too. On hover, the three dot should probably not be seen over the avatar, no?

I think I don't really understand. When the menu is closed, and one hovers the avatar, the three dots icon shows up. This is how it is now already 🤔

The contrast is quite problematic. It should be full background colour with the three dot svg on top of it?

I don't know where this gif comes from, but I think there is something off with it. For me, the three dots have far more contrast, in an app and in the docs:

Screenshot 2023-05-09 at 09-32-43 Nextcloud Vue Style Guide

@skjnldsv
Copy link
Contributor

skjnldsv commented May 9, 2023

I think I don't really understand. When the menu is closed, and one hovers the avatar, the three dots icon shows up. This is how it is now already thinking

You're right, I guess it's because I have a dark theme and it's much easier to see the three dot icon there than the light theme tbh.
We can merge as is

ShGKme
ShGKme approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Works fine now

src/components/NcAvatar/NcAvatar.vue Show resolved Hide resolved
src/components/NcAvatar/NcAvatar.vue Show resolved Hide resolved
src/components/NcAvatar/NcAvatar.vue
Comment on lines +733 to +741
:deep() {
.button-vue,
.button-vue__icon {
height: var(--size);
min-height: var(--size);
width: var(--size) !important;
min-width: var(--size);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a size/small prop to NcButtoninstead of deep internal classes overrides?

@raimund-schluessler raimund-schluessler merged commit 2fe09c1 into master May 10, 2023
16 checks passed
@raimund-schluessler raimund-schluessler deleted the fix/noid/avatar-actions branch May 10, 2023 16:33
This was referenced May 10, 2023
Merged
Merged
@nickvergessen nickvergessen mentioned this pull request May 12, 2023
Closed
This was referenced Jun 17, 2023
Closed
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

@ShGKme ShGKme ShGKme approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@marcoambrosini marcoambrosini marcoambrosini approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@Pytal Pytal Awaiting requested review from Pytal

Assignees
No one assigned
Labels
3. to review Waiting for reviews design Design, UX, interface and interaction design feature: actions Related to the actions components feature: avatar Related to the avatar component feature: popover Related to the popovermenu component technical debt
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Avatar menu alignment and sizing troubles Avatar popover glitch when opened the first time
6 participants
@raimund-schluessler @skjnldsv @nickvergessen @ShGKme @marcoambrosini @artonge