Markdown Links: Fix redirection href values #666
Conversation
There was a problem hiding this comment.
LGTM code wise 👍
I would like to point out a possible side effect that we missed out, which is that our newly defined rendering behavior does not change the underlying body of the issue.
This means that while students are able to use the link on CATcher, they will not be able to do so if they choose the view the issue on GitHub.
So while this PR saves CATcher from hard crashes, it also introduces behavior that is inconsistent with the GitHub comment editor. This can be solved by applying the changes directly to the issue body. Perhaps it is worth discussing the desirability of these changes @CATcher-org/2021-devs
src/app/shared/lib/marked.ts
Outdated
| return linkRenderer.call(renderer, href, title, text); | ||
| } else { | ||
| const html = linkRenderer.call(renderer, href, title, text); | ||
| console.log(html); |
There was a problem hiding this comment.
I think this should be removed or logged with our logger instead of the console log
There was a problem hiding this comment.
I think I forgot to remove the debug statement my bad 😓
|
As discussed, I have made the changes and updated the PR! Do let me know if there is anything I have missed out! >< @dingyuchen @kkangs0226 |
Codecov Report
@@ Coverage Diff @@
## master #666 +/- ##
==========================================
- Coverage 69.08% 68.99% -0.09%
==========================================
Files 76 76
Lines 2274 2274
Branches 208 208
==========================================
- Hits 1571 1569 -2
Misses 660 660
- Partials 43 45 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #666 +/- ##
==========================================
+ Coverage 69.90% 69.92% +0.01%
==========================================
Files 79 80 +1
Lines 2369 2377 +8
Branches 215 215
==========================================
+ Hits 1656 1662 +6
Misses 668 668
- Partials 45 47 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for the PR @seanlowjk - I just have a suggestion that we could add a test for this fix
|
@anubh-v @ptvrajsk @dingyuchen have added a simple test, do let me know if there are changes to be made! |
Summary
This PR fixes #663
Description
When a user generated an invalid link using one of the two methods below, a error gets thrown and the app entirely crashes despite of the platform being used. (Desktop or Web Version). The possible errors generated are from either one of these two 'invalid' links
[Link](invalidlink), an invalid link[Link](www.google.com), a valid link without thehttporhttpsheaderFix Done
As a result, we would need to override the rendered in
MarkedRendererfromngx-markdownsuch that we able to achieve the functionality as below:Simply open the new link in a new tab and propagate the error there. As a result, current progress would not be lost.
Suggested Commit Message: