Skip to content

Conversation

@MarcelRobitaille
Copy link
Collaborator

Fixes #905

When refactoring they keydown handler of EditInputGroup, a regression was introduced where the # would not be inserted when linking to another recipe. This is because we previously had a keyup event listener, which cannot prevent the key from being insterted, but now we are using a keydown listener, which can. Therefore, the e.preventDefault() that was already present in the code now has unintended side effects. We can remove it, there is no reason to have it there.

When refactoring they keydown handler of EditInputGroup,
a regression was introduced where the # would not be inserted when
linking to another recipe. This is because we previously had a keyup
event listener, which cannot prevent the key from being insterted, but
now we are using a keydown listener, which can. Therefore, the
`e.preventDefault()` that was already present in the code now has
unintended side effects. We can remove it, there is no reason to have it
there.

Signed-off-by: Marcel Robitaille <[email protected]>
@codecov
Copy link

codecov bot commented Mar 12, 2022

Codecov Report

Merging #914 (404e36e) into master (7610186) will not change coverage.
The diff coverage is n/a.

❗ Current head 404e36e differs from pull request most recent head 48b13d9. Consider uploading reports for the commit 48b13d9 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #914   +/-   ##
=======================================
  Coverage   20.20%   20.20%           
=======================================
  Files          20       20           
  Lines        1440     1440           
=======================================
  Hits          291      291           
  Misses       1149     1149           
Flag Coverage Δ
integration 5.90% <ø> (ø)
unittests 14.30% <ø> (ø)

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

@github-actions
Copy link

github-actions bot commented Mar 12, 2022

Unit Test Results

  20 files    20 suites   8m 4s ⏱️
  66 tests   66 ✔️ 0 💤 0
660 runs  660 ✔️ 0 💤 0

Results for commit 48b13d9.

♻️ This comment has been updated with latest results.

@christianlupus
Copy link
Collaborator

No, I cannot confirm this to be a fix. At least on my machine (Archlinux Firefox) this triggers a bit of a different behavior: When I type a # in on of the InputGroupFields, a popup opens. This list in the popup is empty. The reason is that the # is prefilled in the search field. Removing it allows to select and add a link. Still, however, the # is not inserted in the final result. So, I still get r/1234 instead of #r/1234.

@christianlupus
Copy link
Collaborator

christianlupus commented Mar 13, 2022

I made a screencast to show my problems. This is different for you?

2022-03-13.16-37-48.mp4

I tested it with firefox + chrome on Win 10 using Browserstack. The same effect happens as in the video.

@MarcelRobitaille
Copy link
Collaborator Author

That is very strange. I can reproduce this. It seems that the contenteditable markdown editor behaves differently to input and textarea. I did not understand the extent of the issue, sorry.

MarcelRobitaille and others added 3 commits March 13, 2022 16:06
The previous fix of switching to keydownwas not viable. The # would
appear in the popup instead of in the main input. This would show no
results in the popup since none have #, and would insert the wrong thing
into the main input.

Fix this by reverting the linking event handler back to keyup. This has
fewer unintended consequences. I also think that the flow is cleaner now
that enter and # are handled in two separate functions.

Signed-off-by: Marcel Robitaille <[email protected]>
Signed-off-by: Christian Wolf <[email protected]>
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.

Seems good enough to me as a fix. LGTM

@christianlupus christianlupus merged commit a19daa5 into nextcloud:master Mar 19, 2022
@MarcelRobitaille MarcelRobitaille deleted the 905-fix-sharp-when-linking branch March 19, 2022 16:33
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.

Fix the linking of recipes in tools/ingredients/instructions

2 participants