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

add new button component #31397

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Conversation

vanpertsch
Copy link
Contributor

@vanpertsch vanpertsch commented Mar 2, 2022

Before
Screenshot 2022-03-02 at 15 23 24

After
Screenshot 2022-03-02 at 15 22 05

@vanpertsch vanpertsch added the 2. developing Work in progress label Mar 2, 2022
@vanpertsch vanpertsch added this to the Nextcloud 24 milestone Mar 2, 2022
@vanpertsch vanpertsch self-assigned this Mar 2, 2022
@vanpertsch vanpertsch mentioned this pull request Mar 2, 2022
18 tasks
@vanpertsch vanpertsch force-pushed the fix/31237/new-button-SetStatusModal branch from 9ced582 to a441c94 Compare March 2, 2022 14:10
@vanpertsch
Copy link
Contributor Author

/compile amend /

Signed-off-by: Vanessa Pertsch <vanessa.pertsch@nextcloud.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

<3

@@ -70,7 +77,7 @@
import { showError } from '@nextcloud/dialogs'
import EmojiPicker from '@nextcloud/vue/dist/Components/EmojiPicker'
import Modal from '@nextcloud/vue/dist/Components/Modal'

import ButtonVue from '@nextcloud/vue/dist/Components/Button'
Copy link
Member

Choose a reason for hiding this comment

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

Why the renaming here? Is it conflicting with other native button components? Should we rename the component itself @skjnldsv?

@jancborchardt
Copy link
Member

Looks good generally, but – @marcoambrosini it does look a bit weird that a clear/delete button is primary-colored, right?
And the full-on red "Error" button would be a bit much, so here and for other cases (e.g. decline attendance for an event), a less aggressive "Delete" button would be good:
image
This uses var(--color-error) as text color, and 10% of that color for background, just like we do for the secondary buttton.

@marcoambrosini
Copy link
Member

@jancborchardt don't you think we can just use tertiary there?
Tbh I think we have plenty of button states for now and for the sake of standardisation I'd prefer to tweak them instead of adding more states. If you think that the "error" state is too punchy we can make it into the one you've posted, but I would advise against having a "light error" alongside a "punchy error"

@vanpertsch
Copy link
Contributor Author

@jancborchardt can we use tertiary here?

Screenshot 2022-03-10 at 16 10 43

@jancborchardt
Copy link
Member

@vanpertsch @marcoambrosini tertiary looks good! :)

@marcoambrosini and yes, I'd say error should be less punchy, at least when we use it for remove, delete and "decline event". Not sure about " Leave call", maybe that needs to stay more obvious as it's the only main action in the case.

@vanpertsch vanpertsch added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Mar 15, 2022
@vanpertsch vanpertsch merged commit 2469640 into master Mar 15, 2022
@vanpertsch vanpertsch deleted the fix/31237/new-button-SetStatusModal branch March 15, 2022 07:11
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants