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 contacts popover #4374

Merged
merged 11 commits into from
Apr 26, 2017
Merged

add contacts popover #4374

merged 11 commits into from
Apr 26, 2017

Conversation

georgehrke
Copy link
Member

@georgehrke georgehrke commented Apr 18, 2017

adds popover as discussed in #207

@georgehrke georgehrke added the 2. developing Work in progress label Apr 18, 2017
@georgehrke georgehrke force-pushed the contactsmenu_popover branch 4 times, most recently from b50f55c to d172faa Compare April 18, 2017 17:33
@MorrisJobke
Copy link
Member

adds popover as discussed in #207

And what about #3233?

@MorrisJobke
Copy link
Member

cc @ChristophWurst

@georgehrke
Copy link
Member Author

This is the popover you will see when clicking on the avatar / name of the user.
#3233 is about the contacts menu in the upper right corner.

#3233 is ready to review while this is still in development, that's why I created a dedicated PR

@MorrisJobke MorrisJobke mentioned this pull request Apr 21, 2017
8 tasks
@jancborchardt
Copy link
Member

@georgehrke there are a bunch of conflicts with master now seemingly, could you resolve them? :)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Tested this in two scenarios where it doesn’t work:

  • in the sharing section of the sidebar, clicking a name does nothing
  • Same for the Activity app, clicking a name in the stream does nothing.

@georgehrke how to test this?

@georgehrke
Copy link
Member Author

in the sharing section of the sidebar, clicking a name does nothing

That should work, what exactly did you test?

Same for the Activity app, clicking a name in the stream does nothing.

Requires changes in the activity app. Will not be part of this pr

@jancborchardt
Copy link
Member

@georgehrke a file shared with another internal user. Should bring up at least info that there is no contact info available, for example. Otherwise it’s confusing. :)

@georgehrke
Copy link
Member Author

kapture 2017-04-24 at 18 50 56

@georgehrke
Copy link
Member Author

add contacts popover by georgehrke pull request 4374 nextcloud-server safari today at 6 52 03 pm

@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 24, 2017
@georgehrke
Copy link
Member Author

@schiessle Can you test this especially with regard to remote sharing please? :) Thx!

@georgehrke georgehrke changed the base branch from contactsmenu to master April 26, 2017 07:29
Signed-off-by: Georg Ehrke <[email protected]>
@georgehrke georgehrke force-pushed the contactsmenu_popover branch from 70ebe0e to 6bbc682 Compare April 26, 2017 07:30
@jospoortvliet jospoortvliet dismissed jancborchardt’s stale review April 26, 2017 08:07
  1. works for me. 2. not in scope for this PR
@jancborchardt
Copy link
Member

Some drone failures – @georgehrke can you fix them?

cc @nextcloud/javascript @MorrisJobke @schiessle

@georgehrke
Copy link
Member Author

»ERROR: maximum time limit exceeded, build cancelled« is not in my power to fix

@jancborchardt
Copy link
Member

Is there a way to restart @MorrisJobke @LukasReschke?

@jospoortvliet
Copy link
Member

jospoortvliet commented Apr 26, 2017

@georgehrke the other error, though, seems a css thingy: https://drone.nextcloud.com/nextcloud/server/7482/33 an element is not visible. Not sure if that's caused by you but I do notice an issue:
screenshot_20170426_1328192
screenshot_20170426_1333312
So this user, doglover, has no email set, neither does the other user that has no actions.

It might be that at some point I added an email to doglover and removed it but I'm not sure. Doglover also shows up with an email sign in the contacts menu by @ChristophWurst with no mouse-over (as there is actually no mail address).

@georgehrke
Copy link
Member Author

https://drone.nextcloud.com/nextcloud/server/7482/33

I have absolutely no clue what this test is about. Where is it defined? Can someone point me?

So this user, doglover, has no email set, neither does the other user that has no actions.
It might be that at some point I added an email to doglover and removed it but I'm not sure.

Can you reproduce the same issue in the contacts menu in the top right corner?

@georgehrke
Copy link
Member Author

https://github.com/nextcloud/server/blob/master/tests/acceptance/features/app-files.feature#L31
And I see that a wrong password for the shared file message is shown

I didn't touch any public sharing code. Not sure that's related to my changes

@MorrisJobke
Copy link
Member

The drone server is highly overloaded causing the timeouts - let me remove some workers. Regarding the acceptance tests: the problem there is also due to the high load -.-

@georgehrke
Copy link
Member Author

Doglover also shows up with an email sign in the contacts menu by @ChristophWurst with no mouse-over (as there is actually no mail address).

Any clue what causes this @ChristophWurst ?

@jancborchardt
Copy link
Member

I think @ChristophWurst is at uni today so we need to fix this without him.

@georgehrke
Copy link
Member Author

georgehrke commented Apr 26, 2017

I think @ChristophWurst is at uni today so we need to fix this without him.

I haven't been able to tackle the bug so far. As it was introduced with the already merged #3233 and not with this PR I'd vote to fix it in a follow up PR (and potentially beta 2)

@LukasReschke
Copy link
Member

I agree with @georgehrke here.

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

Not a critical bug => Let's get this in.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@jancborchardt
Copy link
Member

Who will make the required changes to also make this work in other main shipped apps like:

And sooner or later also:

cc @georgehrke would be cool if you can drive that.

@jancborchardt
Copy link
Member

I also restarted the tests on Drone.

@LukasReschke
Copy link
Member

I also restarted the tests on Drone.

Drone is overloaded. The previous test execution was okay if you ignore the other failing tests. => Let's just merge instead of waiting 10 hours until the Drone backlog is done. Also adding more won't help 🙈

@LukasReschke LukasReschke merged commit d89c760 into master Apr 26, 2017
@LukasReschke LukasReschke deleted the contactsmenu_popover branch April 26, 2017 16:31
@jancborchardt
Copy link
Member

kk :)

@jancborchardt
Copy link
Member

Opened a new issue about adding the contacts popover to other apps at #4532 – would be cool if you can push this @georgehrke, especially for Activity and Gallery as they are shipped.

@nickvergessen
Copy link
Member

I can add it to activities if sample code is posted somewhere

@georgehrke
Copy link
Member Author

I can add it to activities if sample code is posted somewhere

I will send a pr tomorrow :)

@nickvergessen
Copy link
Member

Please also create a topic in https://help.nextcloud.com/c/dev/ so app devs get notified

@ChristophWurst
Copy link
Member

I can add it to activities if sample code is posted somewhere

nextcloud/contacts#173 and https://github.com/ChristophWurst/contactsmenu_social

@ChristophWurst
Copy link
Member

Ah, you mean the popover I guess. Nvm then …

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 Design, UI, UX, etc. enhancement high integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants