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

Remove delete action from list, fix #1368 #1430

Merged
merged 1 commit into from
Jan 23, 2020
Merged

Conversation

jancborchardt
Copy link
Member

Simply removes the delete icons in the contact list as mentioned by the issue at #1368 and illustrated in the screenshot. :)

Deleting a contact is still possibly through the 3-dot menu in the detail view.

Please review @skjnldsv @hanzi @ralxxx

@jancborchardt jancborchardt added enhancement New feature or request 3. to review Waiting for reviews design Related to the design labels Jan 19, 2020
@codecov
Copy link

codecov bot commented Jan 19, 2020

Codecov Report

Merging #1430 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1430   +/-   ##
=======================================
  Coverage   62.31%   62.31%           
=======================================
  Files           4        4           
  Lines          69       69           
=======================================
  Hits           43       43           
  Misses         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32a7d9d...ee31758. Read the comment docs.

Copy link
Member

@hanzi hanzi left a comment

Choose a reason for hiding this comment

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

I like it in principle. That delete button wasn't really necessary there.

But I'm a bit worried that the delete button might be hidden away a bit too much now. That 3-dot menu is far away from everything else. Not sure if it's that obvious to find.

The Mail app (which has the same layout structure) has a second instance of the 3-dot menu in the mail list. Maybe that's an option for Contacts as well?

@hanzi
Copy link
Member

hanzi commented Jan 20, 2020

Another thing: There's still some related CSS code in css/ContactsListItem.scss which should also be removed.

@skjnldsv skjnldsv added this to the next milestone Jan 21, 2020
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member Author

Another thing: There's still some related CSS code in css/ContactsListItem.scss which should also be removed.

Thanks for the catch – fixed. :)

But I'm a bit worried that the delete button might be hidden away a bit too much now. That 3-dot menu is far away from everything else. Not sure if it's that obvious to find.

Agree that the 3-dot-menu is too far away. This is caused by the fact that we go wide into oblivion. We should also restrict the width, just like we do for Mail (
nextcloud/mail#2180 ) and Talk.
But this is something for a separate pull request. :)

The Mail app (which has the same layout structure) has a second instance of the 3-dot menu in the mail list. Maybe that's an option for Contacts as well?

While it has the same layout, the big difference between Mail and Contacts is that Mail is an active app – you are likely in there every day, and working a lot with the entries. Contacts is more of a passive app, where you edit stuff once in a while, and don’t delete as often as you do mails. So I’d say there’s no need for the 3-dot menu in the contact list.

@jancborchardt jancborchardt requested a review from hanzi January 23, 2020 02:51
@hanzi
Copy link
Member

hanzi commented Jan 23, 2020

While it has the same layout, the big difference between Mail and Contacts is that Mail is an active app – you are likely in there every day, and working a lot with the entries. Contacts is more of a passive app, where you edit stuff once in a while, and don’t delete as often as you do mails. So I’d say there’s no need for the 3-dot menu in the contact list.

Valid point, but personally I'd be in favour of having consistent patterns across theses apps. If it looks the same, it should work the same.

But you're right, this is probably better suited as a task of its own.

@skjnldsv skjnldsv merged commit 11d448f into master Jan 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the design/delete-action branch January 23, 2020 09:46
@skjnldsv skjnldsv modified the milestones: next, 3.1.7-3.1.9 Feb 26, 2020
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 Related to the design enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants