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

No background for tertiary-no-background NcActions #3724

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Feb 6, 2023

This removes the background for open NcActions of type tertiary-no-background.

Closes #3720.

Type Before After
tertiary Screenshot 2023-02-06 at 10-09-44 Nextcloud Vue Style Guide Screenshot 2023-02-06 at 10-06-51 Nextcloud Vue Style Guide
tertiary-no-background Screenshot 2023-02-06 at 10-10-43 Nextcloud Vue Style Guide Screenshot 2023-02-06 at 10-07-20 Nextcloud Vue Style Guide

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler added this to the 7.6.0 milestone Feb 6, 2023
@raimund-schluessler raimund-schluessler changed the title No background for tertiary-no-background NcActions No background for tertiary-no-background NcActions Feb 6, 2023
@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews feature: actions Related to the actions components design Design, UX, interface and interaction design labels Feb 6, 2023
@nimishavijay
Copy link

Looks good! One question: Are the 3 states (normal, hover and active/focus) well defined for this type of button? In the deploy-preview component there seems to be no hover state? Or is that an unrelated issue? :)

@raimund-schluessler
Copy link
Contributor Author

Looks good! One question: Are the 3 states (normal, hover and active/focus) well defined for this type of button? In the deploy-preview component there seems to be no hover state? Or is that an unrelated issue? :)

This PR does not touch the NcButton component. So if you mean that an NcButton of type tertiary-no-background has no hover effect (neither text-color or background or similar), that's unrelated.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Thank you!

@nimishavijay
Copy link

Oh sorry, you are right, I meant NcButton
But seems like the NcActions component also doesn't have a hover state? There should be an explicit hover state indicated by, for eg. opacity of the icon :)

@raimund-schluessler
Copy link
Contributor Author

Oh sorry, you are right, I meant NcButton But seems like the NcActions component also doesn't have a hover state? There should be an explicit hover state indicated by, for eg. opacity of the icon :)

That's correct. This is because NcActions uses NcButton internally. So unless we explicitly overwrite it (which we did for e.g. the background), the actions inherit the behaviour of the button. But this (missing) hover effect of a tertiary-no-background button is also not touched here 😉

I would propose to discuss/fix this in a separate issue/PR, see #3727.

@raimund-schluessler
Copy link
Contributor Author

@nimishavijay It seems a hover feedback for tertiary-no-background was not desired, see #3727 (comment). Can we then merge this PR here now, removing the background-color?

@nimishavijay
Copy link

Yes sounds good!

@raimund-schluessler raimund-schluessler merged commit 115b733 into master Feb 13, 2023
@raimund-schluessler raimund-schluessler deleted the fix/3720/tertiary-actions branch February 13, 2023 07:55
@Pytal Pytal mentioned this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UX, interface and interaction design feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NcActions with tertiary-no-background has background when open
4 participants