-
Notifications
You must be signed in to change notification settings - Fork 198
Feat: add cancel button for contact editing #4957
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
Conversation
hamza221
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the Pr,
It looks good to me aside from one point,
On creating a new contact I would expect Cancel to cancel the creation of the contact aka delete the contact.
Maybe we should not show the button for contacts that are just created ? what do you think ?
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I agree that the cancel button could be misunderstood when editing newly created contacts. I removed it in this case. |
hamza221
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from a couple questions
Signed-off-by: Maximilian Martin <[email protected]>
f82b0a1 to
a2d4b5e
Compare
|
I only squashed the commits |
|
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/contacts/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
|
@hamza221 Thanks for reviewing an merging! Should this be backported? |
We usually don't backport features, but good news this is already pre-released v8.3.0-rc.1 |
Fixes #4538
