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

Added tag/keyword cloud to recipe view #373

Merged

Conversation

seyfeb
Copy link
Collaborator

@seyfeb seyfeb commented Nov 5, 2020

Added list of keywords below the title in the recipe view. Klicking keyword routes to recipe list with corresponding tag.

Closes #247

Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
@christianlupus
Copy link
Collaborator

I just looked at your code. I found that the code uses the endpoint /api/search/<tag>. This is the general search query. So, if the tag was found e.g. in another recipe name that recipe will be returned as well.

Was this done by intention or just by chance?

If not done by intention, you might need to register a corresponding search functionality in the search component. The backend needs to be adopted as well (registering a route, adding a controller method and implementing the search itself).

@seyfeb
Copy link
Collaborator Author

seyfeb commented Nov 6, 2020

Good spot, I figured the /tag/X endpoint searches for the keyword. This will require a little bit more work then, so I need to have a look at this later. This PR is WIP then!

@seyfeb seyfeb changed the title Added tag/keyword cloud to recipe view WIP: Added tag/keyword cloud to recipe view Nov 6, 2020
@seyfeb seyfeb marked this pull request as draft November 6, 2020 15:15
@christianlupus
Copy link
Collaborator

I just opened a PR against your fork @seyfeb with the backend work done (at least until the point I assume it is working, testing needs to be done).

@seyfeb
Copy link
Collaborator Author

seyfeb commented Nov 7, 2020

I already did this in parallel, although I went a slightly different route by not using the /tag/ endpoint and instead introduced a new /tags/ endpoint which takes a comma-separated list of tags which must all exist in a recipe to be returned ("AND" operation).

This allows for single tag searches but is more flexible.

Edit: We could provide both, though.

@christianlupus
Copy link
Collaborator

OK, I'll let you do your work until you are done.

BTW: These lines might have an issue with the colon in the parameter's name.

Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
@seyfeb seyfeb force-pushed the feature/showTagCloudInRecipeView branch from 336646b to dfc4839 Compare November 7, 2020 15:27
@seyfeb
Copy link
Collaborator Author

seyfeb commented Nov 7, 2020

I’m glad about any help! ;)
Should we support your endpoint, too? Then I would merge your PR.
Apart from that, I think this PR should be more or less complete then. Depending on your answer I would remove the WIP state.

@christianlupus
Copy link
Collaborator

I already closed the PR. I think the API should be kept as clean as possible, aka drop my suggestion, please.

@seyfeb seyfeb changed the title WIP: Added tag/keyword cloud to recipe view Added tag/keyword cloud to recipe view Nov 7, 2020
@seyfeb seyfeb marked this pull request as ready for review November 7, 2020 18:37
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
@christianlupus christianlupus requested a review from sam-19 November 8, 2020 14:31
@christianlupus
Copy link
Collaborator

I requested @sam-19 as requestor as I am not very well suited for vue.js currently. I will look at the code soon as well.

Copy link
Collaborator

@christianlupus christianlupus left a comment

Choose a reason for hiding this comment

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

The PHP part seems good. I have two comments regarding Vue.JS but I am no expert in thsi field.

src/router/index.js Outdated Show resolved Hide resolved
src/components/AppControls.vue Outdated Show resolved Hide resolved
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
@christianlupus
Copy link
Collaborator

Thank you for the contribution, @seyfeb. Now we can only wait for @sam-19.

Signed-off-by: Sebastian Fey <info@sebastianfey.de>
@sam-19
Copy link
Contributor

sam-19 commented Nov 10, 2020

No issues that I can see, good job! I have avoided using $emit to parent component, because I had some lingering hopes about porting my Vue apps using Vue Native until recently. But since that will probably never happen, using emits is "better" Vue programming and I'll start adopting them more myself.

@christianlupus christianlupus merged commit d8807dd into nextcloud:master Nov 10, 2020
@seyfeb seyfeb deleted the feature/showTagCloudInRecipeView branch November 10, 2020 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show keywords in recipe
3 participants