Fix text box failing to Linkify paragraph children

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

cdkushni
Review Board
release-3.0.x
4685
6888634...
reviewboard, students

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.

Loading file attachments...

  • 0
  • 0
  • 10
  • 0
  • 10
Description From Last Updated
cdkushni
cdkushni
david
  1. 
      
  2. 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. 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. 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. Blank line between these.

  5. 
      
cdkushni
brennie
  1. 
      
  2. 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. 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.

  3. 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.

  4. 
      
cdkushni
cdkushni
brennie
  1. 
      
  2. 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. nit: Your summary doesn't need a period.

  3. 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...