Show tag cloud in search results #375

seyfeb · 2020-11-08T13:32:28Z

Regarding #188.

Added tag cloud between the breadcrumb nav and the recipe overview grid. What it currently does, is

show all keywords of the displayed recipes
keywords are selectable
selecting one or multiple keywords shows only recipes in the current category/search that have all keywords attached

Things to consider

I changed the backend in such a way that not only the recipe name and id are returned anymore, but also a comma-separated string of keywords. The idea is to prevent querying the keywords of each recipe but only having a single database call.
I wasn’t able to get a GROUP_CONCAT working with the QueryBuilder. This might be more efficient than my workaround, so if anyone knows how to get this working - feel free to contribute!

Feedback, as always, welcome.

christianlupus

The backend code looks good to me. There is no way to use GROUP_CONCAT as we are not based on MySQL but a generic SQL driver.
Regarding the Vue.JS I cannot confirm everything as I am not an expert.

One thing I found during testing the functionality:
When one filters the search result by keyword further, it is possible to generate an empty result. This might be surprising for the user. We might want to hide all keywords that are no longer present in the currently filtered view. However, I suggest to keep the current behavior and open an issue/additional PR to solve that bug. Otherwise we might never finish this PR.

christianlupus · 2020-11-08T14:59:49Z

src/components/AppIndex.vue

+                            }
+                        })
+                    }                    
+                })


This will create a huge list of keywords to filter for. If there are many recipes visible, this might be more then fitting in one row. I have not tested but we might have to think of this.

We might need to introduce a scroll view at some point, but for now this should be fine

That will depent how many recipes you have in your cookbook. Some pages set a certain amount of keywords to the tag per recipe. Of course, there will be similarities but for a few 100 recipes the amount of keywords might become significant. But you are right, this might be ok if done later.

christianlupus · 2020-11-08T15:02:06Z

src/components/AppIndex.vue

-ul {
+ul.keywords {
+    display: flex;
+    flex-wrap: wrap;


This will make the comment swap around.

I’m not sure, which comment you mean? What do you mean by swap around?

Sorry, typo. I meant wrap.

Yes, but that’s good, isn’t it? If the keywords don't fit in a single line, the user is still able to see them as in this picture.

christianlupus · 2020-11-08T15:03:32Z

src/components/RecipeKeyword.vue

+li:hover, .active li:hover {
+    border: 1px solid var(--color-primary);
+}
+</style>


This file might conflict with #373.

yes and no. it should be identical 🤞

True, but for the merging we might have a close look for conflicts. No big deal if it's only whitespace or the like.

christianlupus · 2020-11-08T15:17:50Z

I added a rewiew request for @sam-19 as he is our Vue.JS expert.

Thanks @seyfeb for the contribution.

seyfeb · 2020-11-08T21:46:26Z

One thing I found during testing the functionality:
When one filters the search result by keyword further, it is possible to generate an empty result. This might be surprising for the user. We might want to hide all keywords that are no longer present in the currently filtered view. However, I suggest to keep the current behavior and open an issue/additional PR to solve that bug. Otherwise we might never finish this PR.

I ignored your suggestion and implemented your idea of disabling keywords which are not contained in visible recipes!

christianlupus · 2020-11-09T09:37:16Z

Apart from my inability to review the Vue code in detail, it seems to work for me ;-)

sam-19 · 2020-11-10T15:58:35Z

I'm not sure why, but I am not able to test this PR out. When I try to search for recipes, I get the error
Undefined property: OCA\Cookbook\Service\DbCacheService::$user_id at /var/www/html/custom_apps/cookbook/lib/Service/DbCacheService.php
on the following lines:
https://github.com/seyfeb/cookbook/blob/481d7be78ec01fbacc0fceb4b0a2529f613fb7dc/lib/Service/DbCacheService.php#L347
https://github.com/seyfeb/cookbook/blob/481d7be78ec01fbacc0fceb4b0a2529f613fb7dc/lib/Service/DbCacheService.php#L355
https://github.com/seyfeb/cookbook/blob/481d7be78ec01fbacc0fceb4b0a2529f613fb7dc/lib/Service/DbCacheService.php#L383

This in turn triggers an uncaught type error
Trying to access array offset on value of type bool at /var/www/html/custom_apps/cookbook/lib/Db/RecipeDb.php#478

Am I the only one having this problem? Maybe it's something with my setup?

seyfeb · 2020-11-10T19:23:58Z

I don’t think I can reproduce this. Should I see this in the server logs? I’m using @christianlupus docker setup. Where would I have to look? Also, I did not touch the DbCacheService.

sam-19 · 2020-11-10T19:37:01Z

Probably something weird going on with my dev setup. I can see how it works on the index page though and it looks cool :) There are some inconsistencies with line endings in the script sections, but if you wouldn't mind removing those few semicolons here and there, I would definitely give this a thumbs up!

seyfeb · 2020-11-10T19:47:33Z

I removed the semicolons I could find from the updated vue components. Jumping back and forth between languages is annoying 😓

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

seyfeb force-pushed the feature/showTagCloudInSearchResults branch from ead5aea to 5c50b46 Compare November 8, 2020 13:33

christianlupus approved these changes Nov 8, 2020

View reviewed changes

christianlupus requested a review from sam-19 November 8, 2020 15:16

sam-19 approved these changes Nov 10, 2020

View reviewed changes

seyfeb added 7 commits November 12, 2020 12:10

Added keyword list to recipes fetched from database

2c29cb2

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

Added tag cloud to search results

45c77bf

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

Added tag cloud to index

7a82f8f

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

Updated CHANGELOG

7c1bca2

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

Disabling keywords of non-visible recipes in recipe list/grid view

6f6a866

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

Fixed null reference in Search view

2919cad

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

Code cleanup

4ca22cf

Signed-off-by: Sebastian Fey <info@sebastianfey.de>

christianlupus force-pushed the feature/showTagCloudInSearchResults branch from 78bb0f3 to 4ca22cf Compare November 12, 2020 11:13

christianlupus merged commit 7f02208 into nextcloud:master Nov 12, 2020

seyfeb deleted the feature/showTagCloudInSearchResults branch November 16, 2020 13:25

klawdhfzasjhaa mentioned this pull request Dec 10, 2020

Improvements for categories nextcloud/tasks#89

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show tag cloud in search results #375

Show tag cloud in search results #375

seyfeb commented Nov 8, 2020

christianlupus left a comment

christianlupus Nov 8, 2020

seyfeb Nov 8, 2020

christianlupus Nov 9, 2020

christianlupus Nov 8, 2020

seyfeb Nov 8, 2020

christianlupus Nov 9, 2020

seyfeb Nov 9, 2020

christianlupus Nov 8, 2020

seyfeb Nov 8, 2020

christianlupus Nov 9, 2020

christianlupus commented Nov 8, 2020

seyfeb commented Nov 8, 2020

christianlupus commented Nov 9, 2020

sam-19 commented Nov 10, 2020

seyfeb commented Nov 10, 2020

sam-19 commented Nov 10, 2020

seyfeb commented Nov 10, 2020

Show tag cloud in search results #375

Show tag cloud in search results #375

Conversation

seyfeb commented Nov 8, 2020

Things to consider

christianlupus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianlupus commented Nov 8, 2020

seyfeb commented Nov 8, 2020

christianlupus commented Nov 9, 2020

sam-19 commented Nov 10, 2020

seyfeb commented Nov 10, 2020

sam-19 commented Nov 10, 2020

seyfeb commented Nov 10, 2020