Skip to content

Show sharing recommendations #14180

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.

Sign up for GitHub

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

Jump to bottom
Merged
rullzer merged 2 commits into from
Feb 25, 2019
Merged

Show sharing recommendations #14180

rullzer merged 2 commits into from
Feb 25, 2019

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Feb 13, 2019 •
edited
Loading

  • Show recommendation on input focus
  • Client-side logic to show recommendations
  • Server-side logic to query recommendations

Bugs:

  • Does not render the label correctly
  • Might miss some share types
  • Loading spinner shouldn't be active while recommendations are shown
  • Loading spinner appears on second render but does not hide anymore can't reproduce anymore

@ChristophWurst ChristophWurst added this to the Nextcloud 16 milestone Feb 13, 2019
@ChristophWurst ChristophWurst self-assigned this Feb 13, 2019
@ChristophWurst

This comment has been minimized.

Sign in to view
@juliusknorr

This comment has been minimized.

Sign in to view
@juliusknorr juliusknorr mentioned this pull request Feb 19, 2019
Merged
@ChristophWurst

This comment has been minimized.

Sign in to view
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 21, 2019
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Feb 21, 2019

@blizzz please have a look. I think most of this code was originally from you 🙏

Note: there's a bit of duplication going on in the methods. They are very similar and yet slightly different. I don't know the code well enough to refactor it unfortunately, otherwise I would have tried to clean it up.

skjnldsv
skjnldsv approved these changes Feb 21, 2019
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Works great!!
Very nice work @ChristophWurst 👌

The design is off though, but I'm guessing this have nothing to do with this pr ? :)
capture d ecran_2019-02-21_09-07-29

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Feb 21, 2019

The design is off though, but I'm guessing this have nothing to do with this pr ? :)

Nope, I did not change any of the design here but just the behavior ;)

jancborchardt
jancborchardt approved these changes Feb 21, 2019
View reviewed changes
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.

Working nicely! :)

I guess the "Search globally" entry to appear on entry of search terms (to search the global user directory) is separate from this?

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Feb 21, 2019

The design is off though, but I'm guessing this have nothing to do with this pr ? :)

Nope, I did not change any of the design here but just the behavior ;)

I guess the "Search globally" entry to appear on entry of search terms (to search the global user directory) is separate from this?

Yep, this will happen in another PR!

@ChristophWurst ChristophWurst force-pushed the feature/sharing-recommendations branch from 33235cb to 4a2c3ac Compare February 21, 2019 15:07
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Feb 21, 2019

Lots of sh: 1: kill: No such process so I think this should be fine to merge. @juliushaertl @skjnldsv objections?

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 21, 2019
@skjnldsv
Copy link
Member

skjnldsv commented Feb 21, 2019 •
edited
Loading

@ChristophWurst restarted just in case https://drone.nextcloud.com/nextcloud/server/16123

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Feb 21, 2019

And now other tests fail. What does this tell us? 🙈

@rullzer

This comment has been minimized.

Sign in to view
@ChristophWurst ChristophWurst force-pushed the feature/sharing-recommendations branch 2 times, most recently from c25a839 to e596737 Compare February 22, 2019 14:25
@ChristophWurst ChristophWurst mentioned this pull request Feb 22, 2019
Merged
1 task
ChristophWurst and others added 2 commits February 25, 2019 07:25
@ChristophWurst @rullzer
Show sharing recommendations
f3023aa 
Signed-off-by: Christoph Wurst <[email protected]>
@rullzer
Build assets
c484507 
Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer rullzer force-pushed the feature/sharing-recommendations branch from e596737 to c484507 Compare February 25, 2019 06:29
@rullzer rullzer merged commit d48cc08 into master Feb 25, 2019
@rullzer rullzer deleted the feature/sharing-recommendations branch February 25, 2019 08:52
@blizzz
Copy link
Member

blizzz commented Feb 25, 2019

@blizzz please have a look. I think most of this code was originally from you

I only refactored it back in the day, apart of that I don't have much more insights to it than others :)

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Feb 25, 2019

Okay, no problem :)

MorrisJobke
MorrisJobke reviewed Feb 25, 2019
View reviewed changes
apps/files_sharing/appinfo/routes.php
'name' => 'ShareesAPI#findRecommended',
'url' => '/api/v1/sharees_recommended',
'verb' => 'GET',
],
Copy link
Member

@MorrisJobke MorrisJobke Feb 25, 2019

Choose a reason for hiding this comment

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

Please document this in the docs.

Copy link
Member Author

@ChristophWurst ChristophWurst Mar 19, 2019

Choose a reason for hiding this comment

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

Done: nextcloud/documentation#1325

@ChristophWurst ChristophWurst mentioned this pull request Mar 19, 2019
Merged
@ChristophWurst ChristophWurst mentioned this pull request Jun 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke left review comments

@jancborchardt jancborchardt jancborchardt approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@rullzer rullzer Awaiting requested review from rullzer

Assignees

@ChristophWurst ChristophWurst

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement feature: sharing

Projects

None yet

Milestone

Nextcloud 16

Development

Successfully merging this pull request may close these issues.

8 participants

@ChristophWurst @juliusknorr @skjnldsv @rullzer @blizzz @MorrisJobke @jancborchardt