Fix text box failing to Linkify paragraph children

Review Request #10152 — Created Sept. 21, 2018 and submitted

Information

Review Board
release-3.0.x
6888634...

Reviewers

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, …

brenniebrennie

Your description contains your summary as its first line. Also your second line of your description isnt a complete sentence. …

brenniebrennie

The correct format is: Co-authored-by: Stuart Caie <stuart.caie@oracle.com> on a single line and as the last line.

brenniebrennie

You have a typo in the function namespace. "RB.LInkify...." instead of "RB.Linkify....". On that note, when referencing code like that, …

chipx86chipx86

The description is wrapping wayyy too early. Typically, ~72 characters is a good wrapping point here. A good description also …

chipx86chipx86

Youre still wrapping your description/testing done too early (60ch instead of 72). You only need to give credit once.

brenniebrennie

nit: Your summary doesn't need a period.

brenniebrennie

nit: you do not need to mention failing tests on release-3.0.x We generally format what you have in your testing …

brenniebrennie

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 …

daviddavid

Blank line between these.

brenniebrennie
cdkushni
cdkushni
david
  1. 
      
  2. Show all issues

    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) {
        ...
    }
    
  3. 
      
cdkushni
cdkushni
brennie
  1. 
      
  2. Show all issues

    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>
    
  3. Show all issues

    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.

  4. Show all issues

    Blank line between these.

  5. 
      
cdkushni
brennie
  1. 
      
  2. Show all issues

    The correct format is:

    Co-authored-by: Stuart Caie <stuart.caie@oracle.com>
    

    on a single line and as the last line.

  3. 
      
chipx86
  1. 
      
  2. Show all issues

    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.

  3. Show all issues

    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.

  4. 
      
cdkushni
cdkushni
brennie
  1. 
      
  2. Show all issues

    Youre still wrapping your description/testing done too early (60ch instead of 72).

    You only need to give credit once.

  3. 
      
cdkushni
brennie
  1. 
      
  2. Show all issues

    nit: Your summary doesn't need a period.

  3. Show all issues

    nit: you do not need to mention failing tests on release-3.0.x

    We generally format what you have in your testing done as:

    Ran JS tests.
    Ran unit tests.
    

    which implies they passed.

    1. Also, for JS-exclusive changes, you usually don't need to run the Python unit test suite since it doesn't execute any JavaScript.

  4. 
      
cdkushni
david
  1. Ship It!
  2. 
      
cdkushni
  1. Ship It!

  2. 
      
cdkushni
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (7a92207)
Loading...