Skip to content

Conversation

@seyfeb
Copy link
Collaborator

@seyfeb seyfeb commented Jan 28, 2021

Added functionality to reference other recipes by id in description, tools, ingredients, and instructions

  • In recipe editor, when entering '#' a popup with a multiselect is shown that allows selecting a reference.
  • the popup is shown in fullscreen for small window widths
  • When selecting a recipe a reference is entered: #r/123
  • in recipe viewer this is converted to #123 linking to the recipe page

Closes #209

@seyfeb seyfeb requested a review from christianlupus January 28, 2021 20:51
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #562 (ee8521b) into master (6b8cdf0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #562   +/-   ##
========================================
  Coverage      1.01%   1.01%           
  Complexity      440     440           
========================================
  Files            13      13           
  Lines          1381    1381           
========================================
  Hits             14      14           
  Misses         1367    1367           
Flag Coverage Δ Complexity Δ
integration 0.00% <ø> (ø) 0.00 <ø> (ø)
unittests 1.01% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@christianlupus
Copy link
Collaborator

I will have to play with the PR a bit. There are quite some changes so I cannot oversee statically fi this will work.

I have two points seen during a quick look at the code:

  1. Usage if v-html must be used carefully. This allows CSRF attacks if corresponding HTML code is embedded in the corresponding texts. This must be carefully checked and eventually escaped.
  2. This will conflict heavily with Markdown for description #381.

@seyfeb
Copy link
Collaborator Author

seyfeb commented Jan 29, 2021

I will have to play with the PR a bit. There are quite some changes so I cannot oversee statically fi this will work.

Definitely. The main change (in lines of code) is certainly the new component EditMultiselectPopup.vue. In the input fields, registering the # press and pasting the reference is done. In the RecipeView component the references are converted to links.

  1. Usage if v-html must be used carefully. This allows CSRF attacks if corresponding HTML code is embedded in the corresponding texts. This must be carefully checked and eventually escaped.

That is a good point. As Vue does this automatically in most cases I sometimes forget about it. I add some escaping mechanism before inserting it there.

  1. This will conflict heavily with Markdown for description #381.

It conflicts, but I hope™ that it’s easily fixable.

@seyfeb seyfeb force-pushed the feature/issue209referenceRecipes branch from 692514f to deb99fb Compare January 29, 2021 19:21
@seyfeb seyfeb marked this pull request as draft January 30, 2021 19:03
@seyfeb seyfeb force-pushed the feature/issue209referenceRecipes branch from deb99fb to d6ba601 Compare January 31, 2021 14:55
@seyfeb
Copy link
Collaborator Author

seyfeb commented Jan 31, 2021

I merged the Markdown support and rebased / extended this PR on this basis. It should work for both markdown and non-markdown input fields now!

@seyfeb seyfeb marked this pull request as ready for review January 31, 2021 15:03
@seyfeb seyfeb added enhancement New feature or request Frontend Issue or PR related to the frontend code javascript Pull requests that update Javascript code labels Feb 2, 2021
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.

There is one issue with the Changelog that must be tackled and a few minor things that can be taken care of but are mainly border cases.

Apart from these it seems to work and can be merged once these are ironed out.

@seyfeb seyfeb force-pushed the feature/issue209referenceRecipes branch from af5e6a1 to 2d550a7 Compare February 4, 2021 20:44
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.

You might want to have a look at the minor change I suggested but apart from that I think, this can be merged.

seyfeb and others added 2 commits February 5, 2021 15:51
@seyfeb seyfeb force-pushed the feature/issue209referenceRecipes branch from 1f920dd to dbe6b67 Compare February 5, 2021 14:52
@seyfeb seyfeb merged commit f8c10be into master Feb 5, 2021
@delete-merged-branch delete-merged-branch bot deleted the feature/issue209referenceRecipes branch February 5, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Frontend Issue or PR related to the frontend code javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Link to other recipes with hashes

3 participants