Skip to content

Show sharing recommendations#14180

Merged
rullzer merged 2 commits intomasterfrom
feature/sharing-recommendations
Feb 25, 2019
Merged

Show sharing recommendations#14180
rullzer merged 2 commits intomasterfrom
feature/sharing-recommendations

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 <christoph@winzerhof-wurst.at>
@rullzer
Build assets
c484507 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@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.

7 participants

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