Contacts menu by ChristophWurst · Pull Request #3233 · nextcloud/server

ChristophWurst · 2017-01-24T14:42:22Z

Rough todo list:

client-side
- display actions listed by the API
- ~~lazy loading~~ not necessary if number of elements is limited to 25.
- link to contacts app
- search
  - show input
  - query and show results from server
server-side
- ~~discuss API design in separate issue~~ got not feedback within two weeks
- implement API (registration system + some example clients, e.g. in Mail) Example: Add contacts menu integration contacts#173

implements #207

Current screenshot:

ChristophWurst · 2017-02-06T07:59:26Z

Dang it. I've lost my most recent changes due to a rebase+force push of an outdated version of the branch 😠

ChristophWurst · 2017-02-13T07:52:50Z

Dang it. I've lost my most recent changes due to a rebase+force push of an outdated version of the branch 😠

Got them back from my other PC 💪 😌

nickvergessen · 2017-02-20T11:36:45Z

lib/private/Contacts/ContactsMenu/ActionProviderStore.php

+					'message' => "Could not load contacts menu action provider $class",
+					'app' => 'core',
+				]);
+				throw new \Exception("Could not load contacts menu action provider $class");


Don't throw $class to avoid input injection. You log the class, that's fine enough

Makes sense!

LukasReschke · 2017-02-20T11:39:09Z

core/templates/layout.user.php

 			</div>
 		</div>
+
+		<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" type="text/javascript">


Doing this here has some security implications as a XSS in $_['content'] may be able to extract this by using an <img> tag etc.

Can we move that to a dedicated script of before the content? :)

Sure we can. I just wanted to do it inline for performance reasons as we'd save one request per page load.

IMHO better solved by making sure we concatenate all JS files that we deliver anyways on nearly every page load :)

ChristophWurst · 2017-02-20T16:08:58Z

1) Tests\Contacts\ContactsMenu\EntryTest::testAddAndSortAction
usort(): Array was modified by the user comparison function
/drone/src/github.com/nextcloud/server/lib/private/Contacts/ContactsMenu/Entry.php:105
/drone/src/github.com/nextcloud/server/lib/private/Contacts/ContactsMenu/Entry.php:87
/drone/src/github.com/nextcloud/server/tests/lib/Contacts/ContactsMenu/EntryTest.php:112

php5.6+usort+phpunit mocks = 💩

ChristophWurst · 2017-02-20T16:11:27Z

Alright, this has already gotten bigger than I wanted it to be for the first PR. Let's do the more advanced stuff in follow-up PRs:

Avatar from the contacts app
App action provider plugin system
Last interaction

ChristophWurst · 2017-02-20T21:07:59Z

@nextcloud/designers I need some help with styling the different elements:

Should the inner popover menu overlap the scrollbar or should it only be visible inside the menu?

@skjnldsv do you know if I have to/should tweak the css to get the popover menu positioned right or are there any server classes I have to use or guidelines I have to follow? :-)

skjnldsv · 2017-02-20T22:54:07Z

@ChristophWurst #2798

skjnldsv · 2017-02-21T08:01:35Z

Just want to say that it looks damn good! 🦀

skjnldsv · 2017-02-21T08:11:19Z

By the way the other menu on the header don't use a popovermenu.
The yours looks a bit out of place compared to the others :)

skjnldsv · 2017-02-21T12:53:51Z

try to comply with the css guidelines

😂
I will try to help on this after my work :)

jancborchardt · 2017-02-21T14:20:42Z

Heya, answering some of your questions @ChristophWurst

Should the inner popover menu overlap the scrollbar or should it only be visible inside the menu?

It’s fine to overlap the scrollbar, it’s a level over the inner menu. It should also be above the scrollable area because when you open the menu on an entry down at the end it should show properly and not expand inside the scrollable area. If it’s too difficult then for the first iteration we could have it show inside. But we should have some smooth autoscrolling going on maybe which scrolls the menu in view. Not perfect though so it’s preferable to show the menu outside of the container.

@skjnldsv: By the way the other menu on the header don't use a popovermenu.

Yup, we need to adjust the detail positioning here. No space between header and menu, and remove border-radius on the top corners. :)
The icon needs to be also available in white, depending on theme. cc @juliushaertl
The max-height of the popover should be exactly n+1/2 of an entry, so you see when there’s more. Maybe 4 1/2 is best?
On scrolling down, new contacts should be loaded automatically via lazy loading. For the future when you have chats with all these folks I think it’s fine to just have all of it in there without a shortlink to Contacts. You will be able to use search anyway.

Anything else? I guess lots is for a follow-up PR anyway. :)

MorrisJobke · 2017-02-22T00:20:20Z

I fixed the icon color and it now looks a lot nicer:

MariusBluem · 2017-02-22T00:39:42Z

@MorrisJobke Better, but should the item indicate about whether it is opened or not (a bit more grey or something...) 🤔 @jancborchardt

codecov-io · 2017-02-22T01:18:50Z

Codecov Report

Merging #3233 into master will increase coverage by 0.13%.
The diff coverage is 90.85%.

@@             Coverage Diff              @@
##             master    #3233      +/-   ##
============================================
+ Coverage      54.1%   54.23%   +0.13%     
- Complexity    21716    21774      +58     
============================================
  Files          1334     1343       +9     
  Lines         83213    83528     +315     
  Branches       1315     1327      +12     
============================================
+ Hits          45025    45305     +280     
- Misses        38188    38223      +35

Impacted Files	Coverage Δ	Complexity Δ
core/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/templates/layout.user.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/js/js.js	`61.21% <100%> (-0.44%)`	`0 <0> (ø)`
lib/private/legacy/template.php	`30.82% <100%> (+0.47%)`	`36 <0> (ø)`	⬇️
...ib/private/Contacts/ContactsMenu/ActionFactory.php	`100% <100%> (ø)`	`2 <2> (?)`
core/Controller/ContactsMenuController.php	`100% <100%> (ø)`	`2 <2> (?)`
lib/private/Server.php	`93.32% <100%> (+0.01%)`	`120 <0> (ø)`	⬇️
.../Contacts/ContactsMenu/Providers/EMailProvider.php	`100% <100%> (ø)`	`3 <3> (?)`
...vate/Contacts/ContactsMenu/ActionProviderStore.php	`87.5% <87.5%> (ø)`	`9 <9> (?)`
...ivate/Contacts/ContactsMenu/Actions/LinkAction.php	`90% <90%> (ø)`	`8 <8> (?)`
... and 18 more

skjnldsv · 2017-02-22T09:25:05Z

Disagree @MariusBluem . We have the triangle indicator, that's enough I think :)

LukasReschke · 2017-02-22T09:42:17Z

lib/private/Contacts/ContactsMenu/Manager.php

+		return [
+			'contacts' => $topEntries,
+			'contactsAppEnabled' => $contactsEnabled,
+			'contactsAppURL' => $contactsURL,


Will generate a URL without index.php for me which breaks. You should either link to the route directly, or always prepend /index.php/ or just use the JS OC.generateUrl('/apps/contacts'); which should take care of that.

In this scenario I'd probably vote for the JS approach.

LukasReschke · 2017-02-22T09:42:36Z

lib/private/Contacts/ContactsMenu/Providers/DetailsProvider.php

+
+		// TODO: unique contact URL to the contacts app
+		// TODO: l10n
+		$contactsUrl = $this->urlGenerator->getAbsoluteURL('/apps/contacts');


Same as https://github.com/nextcloud/server/pull/3233/files#r102418886

Signed-off-by: Christoph Wurst <[email protected]>

When we test wheter action menus in the contacts menu close when clicking other ones, we have to provide test data that actually causes the view to render the menu. Signed-off-by: Christoph Wurst <[email protected]>

Signed-off-by: Christoph Wurst <[email protected]>

We do not want to have sensitive information in the URL and therefore also not in the access log. Thus the GET request is replaced by a POST request. Signed-off-by: Christoph Wurst <[email protected]>

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

ChristophWurst · 2017-04-25T18:49:16Z

Rebased. Thanks for testing, @jospoortvliet! :)

jospoortvliet · 2017-04-25T19:03:26Z

Tested your rebased version, works too.

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

jancborchardt · 2017-04-25T22:13:38Z

So I

fixed the test (which was failing due to a text change I made)
fixed merge conflicts in PHP files

I think it goes without saying this should be double-checked. 😂

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

ChristophWurst · 2023-10-04T09:36:52Z

@ChristophWurst please document the API in the dev manual

ChristophWurst · 2023-10-17T12:09:31Z

@ChristophWurst please document the API in the dev manual

nextcloud/documentation#11222

ChristophWurst added 2. developing Work in progress design Design, UI, UX, etc. enhancement high integration labels Jan 24, 2017

ChristophWurst added this to the Nextcloud 12.0 milestone Jan 24, 2017

ChristophWurst self-assigned this Jan 24, 2017

ChristophWurst force-pushed the contactsmenu branch from 5af5c16 to 333ef1a Compare February 6, 2017 07:53

MariusBluem mentioned this pull request Feb 7, 2017

Ellipsize user name in top right #3273

Closed

ChristophWurst force-pushed the contactsmenu branch from 333ef1a to 10ea8f7 Compare February 13, 2017 07:45

nickvergessen reviewed Feb 20, 2017

View reviewed changes

LukasReschke reviewed Feb 20, 2017

View reviewed changes

ChristophWurst force-pushed the contactsmenu branch from 8e0c703 to 5dfd133 Compare February 20, 2017 15:54

ChristophWurst mentioned this pull request Feb 21, 2017

[styleguide] Popover menu #2798

Closed

1 task

LukasReschke reviewed Feb 22, 2017

View reviewed changes

ChristophWurst and others added 11 commits April 25, 2017 20:47

Use tooltop for contact's top action title

5762cd9

Signed-off-by: Christoph Wurst <[email protected]>

Let apps register contact menu provider via info.xml

36cee1f

Signed-off-by: Christoph Wurst <[email protected]>

Don't show contacts an entry for themselves

b8c2a8a

Signed-off-by: Christoph Wurst <[email protected]>

Use absolute URI for action icons

2c2e1f7

Signed-off-by: Christoph Wurst <[email protected]>

Fix icon of second action

ecd408d

Signed-off-by: Christoph Wurst <[email protected]>

Fix failing contacts menu js test data

5c61852

When we test wheter action menus in the contacts menu close when clicking other ones, we have to provide test data that actually causes the view to render the menu. Signed-off-by: Christoph Wurst <[email protected]>

Adjust entry unit test to newly added avatar property

98f02fa

Signed-off-by: Christoph Wurst <[email protected]>

Update composer autoloader

5c29fb1

Signed-off-by: Christoph Wurst <[email protected]>

Use HTTP POST to retrieve the list of contacts

945420b

We do not want to have sensitive information in the URL and therefore also not in the access log. Thus the GET request is replaced by a POST request. Signed-off-by: Christoph Wurst <[email protected]>

Fix spinner position of Contacts menu

4ae5340

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

Fix emptycontent HTML and wording

2ccaae8

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

ChristophWurst force-pushed the contactsmenu branch from f4ee854 to 2ccaae8 Compare April 25, 2017 18:47

Fix test

0f0b04b

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

Merge branch 'master' into contactsmenu

241e397

Signed-off-by: Jan-Christoph Borchardt <[email protected]>

jancborchardt force-pushed the contactsmenu branch from a423882 to 241e397 Compare April 25, 2017 22:50

jancborchardt merged commit 6db6911 into master Apr 25, 2017

MorrisJobke mentioned this pull request Apr 26, 2017

Design spec: Contacts menu #207

Closed

jancborchardt deleted the contactsmenu branch April 26, 2017 09:53

sualko mentioned this pull request Apr 26, 2017

Add nc contact menu integration jsxc/jsxc#519

Closed

MariusBluem added the feature: contacts menu label Apr 30, 2017

jancborchardt mentioned this pull request May 2, 2017

Add contacts popover to other apps #4532

Closed

11 tasks

miaulalala mentioned this pull request Apr 26, 2023

Extend contacts menu to show system address book users too nextcloud/contacts#3341

Open

ChristophWurst added the pending documentation This pull request needs an associated documentation update label Oct 4, 2023

ChristophWurst mentioned this pull request Oct 17, 2023

feat(devmanual): Document contacts menu basics nextcloud/documentation#11222

Merged

ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Oct 17, 2023

Uh oh!

Conversation

ChristophWurst commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristophWurst commented Feb 6, 2017

Uh oh!

ChristophWurst commented Feb 13, 2017

Uh oh!

nickvergessen Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

ChristophWurst Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschke Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

ChristophWurst Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschke Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Feb 20, 2017

Uh oh!

ChristophWurst commented Feb 20, 2017

Uh oh!

ChristophWurst commented Feb 20, 2017

Uh oh!

skjnldsv commented Feb 20, 2017

Uh oh!

skjnldsv commented Feb 21, 2017

Uh oh!

skjnldsv commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Feb 21, 2017

Uh oh!

jancborchardt commented Feb 21, 2017 • edited by ChristophWurst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MorrisJobke commented Feb 22, 2017

Uh oh!

MariusBluem commented Feb 22, 2017

Uh oh!

codecov-io commented Feb 22, 2017 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skjnldsv commented Feb 22, 2017

Uh oh!

LukasReschke Feb 22, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschke Feb 22, 2017

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Apr 25, 2017

Uh oh!

jospoortvliet commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jancborchardt commented Apr 25, 2017

Uh oh!

ChristophWurst commented Oct 4, 2023

Uh oh!

ChristophWurst commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

ChristophWurst commented Jan 24, 2017 •

edited

Loading

skjnldsv commented Feb 21, 2017 •

edited

Loading

jancborchardt commented Feb 21, 2017 •

edited by ChristophWurst

Loading

codecov-io commented Feb 22, 2017 •

edited by codecov bot

Loading

jospoortvliet commented Apr 25, 2017 •

edited

Loading