Fix text box failing to Linkify paragraph children
Review Request #10152 — Created Sept. 21, 2018 and submitted
When making a paragraph with multiple newline links in a text box and
hitting 'ok' only the first link would be linkified.This was tested and it was found that the paragraph children were not
handling siblings correctly such that if one is replaced it causes the
loop variable 'node' becomes stale when referencing next siblings.The best method to get around this was to use the fact that all the child
nodes are available to be iterated in order. By doing so we avoid dealing
with pointer references which when replaced were causing the issues.Co-authored-by: Stuart Caie stuart.caie@oracle.com
Ran JS tests.
Ran unit tests.Tested by inputting paragraphs of links in different amounts.
Tested by putting plain text above and below paragraphs of links inclusive
and exclusively with multiple links and singular links.
Description | From | Last Updated |
---|---|---|
Can you rewrite your summary to: 1) be at most 50 characters long 2) be written in the imperitive mood, … |
brennie | |
Your description contains your summary as its first line. Also your second line of your description isnt a complete sentence. … |
brennie | |
The correct format is: Co-authored-by: Stuart Caie <stuart.caie@oracle.com> on a single line and as the last line. |
brennie | |
You have a typo in the function namespace. "RB.LInkify...." instead of "RB.Linkify....". On that note, when referencing code like that, … |
chipx86 | |
The description is wrapping wayyy too early. Typically, ~72 characters is a good wrapping point here. A good description also … |
chipx86 | |
Youre still wrapping your description/testing done too early (60ch instead of 72). You only need to give credit once. |
brennie | |
nit: Your summary doesn't need a period. |
brennie | |
nit: you do not need to mention failing tests on release-3.0.x We generally format what you have in your testing … |
brennie | |
So the suggested fix works, but we can't use for ... of in our code (see https://www.notion.so/reviewboard/JavaScript-ES6-Guidelines-a5d6efb850d24ac295e1b8792cecdd25) Instead, we can … |
david | |
Blank line between these. |
brennie |
Testing Done: |
|
---|
-
-
reviewboard/static/rb/js/utils/linkifyUtils.es6.js (Diff revision 1) So the suggested fix works, but we can't use
for ... of
in our code (see https://www.notion.so/reviewboard/JavaScript-ES6-Guidelines-a5d6efb850d24ac295e1b8792cecdd25)Instead, we can just iterate with an index:
for (let i = 0; i < el.childNodes.length; i++) { const node = el.childNodes[i]; if (node.nodeType === node.TEXT_NODE) { ... }
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+2 -1) |
Checks run (2 succeeded)
Testing Done: |
|
---|
-
-
Can you rewrite your summary to:
1) be at most 50 characters long
2) be written in the imperitive mood, i.e., as if it were a command. If you substitute your summary into the following sentence, it should make sense:This patch will <summary>
e.g., "Fix RB.LinkifyUtils.linkifyChildren on all browsers" or similar.
If you want to credit
kyz
for their work you can add the following to your summary and we will include it as a git trailer when we land:Co-authored-by: Stuart Caie <stuart.caie@oracle.com>
-
Your description contains your summary as its first line. Also your second line of your description isnt a complete sentence.
Give our guide on writing good change descriptions a read and update your description based on that.
-
Summary: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
-
-
You have a typo in the function namespace. "RB.LInkify...." instead of "RB.Linkify....".
On that note, when referencing code like that, you can put a backtick on either side to turn it into a literal.
-
The description is wrapping wayyy too early. Typically, ~72 characters is a good wrapping point here.
A good description also helps people to better understand what was wrong and why and how it was addressed without digging into the code. From yours, I can understand what the problem was, and then I get some technical details that make no sense unless I know this code already. What you can do to improve the description is to give a summary of what went wrong and why, without assuming any prior knowledge. Something like "The reason that only the first line was linked was <...>. By doing <...>, we can properly cover all lines," or something like that.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+3 -1) |
Checks run (2 succeeded)
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Summary: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|