Issue 3201: Put a link next to rich-text fields.

Review Request #5378 — Created Feb. 1, 2014 and submitted

Information

Review Board
master

Reviewers

Appended the markdown link next to the buttons in 'Description', 'Testing Done', 'Comments' and 'Reply to comments' fields.

For each view, checked the state of all combinations of buttons pressed/unpressed to make sure the markdown link appears when beginning edit and disappears when cancelling edit. For reviewReply and reviewRequest views, opened multiple text editors at the same time to check if the markdown informaiton appears.


Description From Last Updated

Can you use CSS to right-align this within its parent? I think I'd also like it if it said something …

daviddavid

Leave this blank line here.

daviddavid

Instead of using to split this across lines, I think I'd prefer joining an array of strings: $('.body-top').append([ '<a class="markdown-info" …

daviddavid

Same here with string joins.

daviddavid

This line has trailing whitespace.

daviddavid

Please put a space between if and (.

daviddavid

Same here with string joins.

daviddavid

Please put a space between if and (.

daviddavid

Please put a space between if and (.

daviddavid

This should be indented two spaces, and have a space between the : and the value.

daviddavid

Leave this line here.

daviddavid

These look like they're indented 3 spaces instead of 4.

daviddavid

The strings should be indented 4 spaces in from the previous alignment. It might be worth moving all the chained …

daviddavid

Same here with indentation and spaces at the ends of each string.

daviddavid
david
  1. Please attach screenshots of the changed UIs.

    You also have several places in your code that use tabs. We only use spaces for indentation.

    1. Made the required changes

  2. 
      
B.
david
  1. This is looking better.

    I didn't see code or screenshots for doing this to the comment dialog. Can you also show a screenshot of the review dialog with multiple comments in the draft?

    1. Please also but the bug number into the "Bugs" field.

  2. Show all issues

    Can you use CSS to right-align this within its parent?

    I think I'd also like it if it said something like "This field uses Markdown" instead of just "Markdown Field".

  3. Show all issues

    Instead of using to split this across lines, I think I'd prefer joining an array of strings:

    $('.body-top').append([
        '<a class="markdown-info" ',
        '   href="..." ',
        '   target="_blank">...</a>'
        ].join(''));
    
  4. Show all issues

    Same here with string joins.

  5. Show all issues

    This line has trailing whitespace.

  6. Show all issues

    Same here with string joins.

  7. 
      
david
  1. 
      
  2. Show all issues

    Leave this blank line here.

  3. Show all issues

    Please put a space between if and (.

  4. Show all issues

    Please put a space between if and (.

  5. Show all issues

    Please put a space between if and (.

  6. 
      
B.
B.
david
  1. 
      
  2. reviewboard/static/rb/css/reviews.less (Diff revision 3)
     
     
    Show all issues

    This should be indented two spaces, and have a space between the : and the value.

  3. reviewboard/static/rb/css/reviews.less (Diff revision 3)
     
     
    Show all issues

    Leave this line here.

  4. reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 3)
     
     
     
     
     
    Show all issues

    These look like they're indented 3 spaces instead of 4.

  5. Show all issues

    The strings should be indented 4 spaces in from the previous alignment.

    It might be worth moving all the chained methods down and indent them only 4 spaces. Something like this:

    $draftComment
        .find('pre.reviewtext')
        .inlineEditor('buttons')
        .append([
            '<a class="markdown-info" '
            ...
        ]);
    

    You also need to add spaces at the ends of these strings, because the join won't insert them. As it is, this would create the string "<a class="markdown-info"href="..."target="...">"

  6. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    Same here with indentation and spaces at the ends of each string.

  7. 
      
B.
david
  1. I'm going to make some small changes (a little bit to the style and adding localization) and push this. Thanks!

  2. 
      
B.
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (997ad1a). Thanks!
Loading...