Addressbooklist in a popover

[skjnldsv]

Testing:
contacts.tar.gz

• Popover structure
  • Css guidelines
  • Don't close settings after click
• Clipboard
  • Tooltips
  • Fallback to input if no clipboard support
• Shares design
  • List
  • Checkboxes
  • Don't close settings after click

Fix and finishes #14

[codecov-io]

Codecov Report

Merging #101 into master will decrease coverage by -0.25%.
The diff coverage is 5.26%.

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
- Coverage   16.31%   16.07%   -0.25%     
==========================================
  Files          49       49              
  Lines         999     1014      +15     
==========================================
  Hits          163      163              
- Misses        836      851      +15

Impacted Files	Coverage Δ
...ents/addressBookList/addressBookList_controller.js	7.14% <ø> (-0.55%)	❌
...s/components/addressBook/addressBook_controller.js	2.63% <ø> (-0.6%)	❌
js/main.js	100% <100%> (ø)	✅

---

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 45c9167...7f1ebfd. Read the comment docs.

[jancborchardt]

Very nice work! Just 3 details which are probably related to the core styles:

• top right border radius is not set
• there is a bit too much bottom padding on the popover
• »Share« and »Delete« are much lighter than the other item. Were you hovering »Download«? Cause they should be the same lightness.

And one thing: Because sharing is inside the menu, »Copy link« should be too. Because otherwise the icon without text will cause confusion because it’s used in the context of sharing usually. Yes, for sharing a link, but mostly in combination with the other sharing. :)

[skjnldsv]

@jancborchardt Thanks! :)

• I wasn't hovering it, there is some fix to do.
• Top right indeed comes form core, will fix.
• Makes sense for the copy link/share. Should we put the download button outside then?

[skjnldsv]

This is what it does on stable 11 @jancborchardt

[skjnldsv]

Backport on nextcloud/server#3286

[MorrisJobke]

For me those icons don't appear in a popover but inline in the row. 😕 So the three dots menu is not there.

[skjnldsv]

@MorrisJobke I added a contact build for testing. :)
But we need this backport! nextcloud/server#3286

[skjnldsv]

Rebased.

We now should wait for 11.0.2

[skjnldsv]

This is an ugly trick. But core is broken for popups in the settings area.
Here is the issue: https://github.com/nextcloud/server/blob/25acce7e0d992b10b930472b8138240b7bd9af13/core/js/apps.js#L108

[skjnldsv]

@nextcloud/designers @nextcloud/contacts: all clear.
Screenshots are updated on first post.

Please review! 🐤
@jancborchardt @irgendwie @Henni

[irgendwie]

@skjnldsv The 'Copy URL' tooltip is a bit glitchy for me. When clicking 'Copy URL' the 'Copied!' tooltip appears shortly in the top left corner of the screen. When hovering the 'Copy URL' (after closing and reopening the settings) the tooltip still shows 'Copied!' - only after some time it changes back to the original help text.

Otherwise LGTM 👍

Edit: @skjnldsv broken on nc 9 (and possibly 10?)

[jancborchardt]

@skjnldsv any update? Would be great if we can get this in time for Nextcloud 12 :)

@irgendwie I would say the tooltip glitch we can fix separately? :) Let’s move forward and not block this PR too long.

[skjnldsv]

@jancborchardt this update is ready to be merged.
Just the tooltip bug @irgendwie had.

[irgendwie]

@jancborchardt Would love to merge, still it would break the contacts app for nc 9 and nc 10 and would only be compatible with nc 11 and 12. Are we willing to do this?

[skjnldsv]

For the record, I am :)

[jancborchardt]

cc @LukasReschke @MorrisJobke @jospoortvliet @Henni @ChristophWurst for second opinions.

I would agree with @skjnldsv it’s ok for the Contacts app to only support the two latest releases. It’s a big testing workload otherwise and also contacts syncing is in core anyway. And the app in its current state is still available for Nextcloud 9 and 10.
We want people to update to new versions also for security reasons, so getting proper updated apps is another push to do so.

[Henni]

@jancborchardt I agree. But we should really support the latest two versions and thus might want to wait for the nc 12 release.

[jancborchardt]

@jancborchardt I agree. But we should really support the latest two versions and thus might want to wait for the nc 12 release.

Yes, good call – do you mean after or before the Nextcloud 12 release? And for when is the next release of the Contacts app planned? Nextcloud 12 will come soon. :)

[Henni]

@jancborchardt how about the same day as Nextcloud 12? 😉

The current milestone has a due date of April 1st. But it might make sense to delay it to the nc release.

[jancborchardt]

@Henni cool, let’s do the Contacts app release on the same day then! :) But then let’s also merge this change already so it can be used and tested as early as possible. :) Ok @irgendwie @Henni @skjnldsv?