• 
      

    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:
    Completed
    Change Summary:
    Pushed to release-3.0.x (7a92207)