[WIP] Inline help text for view diff page

Review Request #10910 — Created Feb. 21, 2020 and updated

xiaole2
Review Board
master
reviewboard

Add inline help text for revision selector, how to make comments, and the expander/collapse button in View Diff page
The inline help text for revision selector will show up when there is a revision selector(which means you need to have more than one version).
The inline help text for expander/collapse buttons will switch based on current button status.

Tested in Chrome, Firedox, Safari

Summary Author
view diff page inline text
XiaoleZ
1. change one-space indentation. 2. edit/update help text more accurate. 3. make style consistent
XiaoleZ
remove trailing space
XiaoleZ
Loading file attachments...

Description From Last Updated

I would swap the order, but any help text above the operations.

chipx86chipx86

We use 1-space indentation in HTML.

chipx86chipx86

I thought I had written this already on your change for rb-c-tip, but I didn't. Oops! I know I started ...

chipx86chipx86

You may have noticed the <%= vs. <%- in the templates. The former will embed the variable as HTML, while ...

chipx86chipx86

This line looks to be too long. It should wrap at < 80 characters.

chipx86chipx86

How about: "Click a line number to comment on that line. To create a multi-line comment, click-and-drag down multiple line ...

chipx86chipx86

I don't feel this tip is very useful as-is. It doesn't say what this actually does, just that you can ...

chipx86chipx86

This should use one-space indentation.

chipx86chipx86

Some things to note: The <p> tag should be on its own line, with content appearing on subsequent lines, indented ...

chipx86chipx86

Text like "You could ..." isn't really ideal for UI text. This also isn't saying how you could select specific ...

chipx86chipx86
xiaole2
xiaole2
  1. 
      
  2. Here is some ideas for reviewers:

    Based on the image files, here is the the location for those inline help text. I wonder would anyone have other preference or opinion about those location?

    For the inline tip text of making comments and clickable expander/collapse button, do you want to centralize the text or left-aligned?

  3. 
      
chipx86
  1. 
      
  2. I would swap the order, but any help text above the operations.

  3. We use 1-space indentation in HTML.

  4. I thought I had written this already on your change for rb-c-tip, but I didn't. Oops! I know I started working on it...

    The rb-c-tip class is probably itself not needed. It's really similar in concept to rb-c-alert, which offers modifier classes for info alerts, warnings, errors, etc.

    I think I'd like to see you use this instead of rb-c-tip, and use rb-c-alert -is-info here. See the documentation for rb-c-alert for the structure.

    This will have a more consistent style that the rest of the UI is going to be using more, and will be more noticeable to users.

  5. You may have noticed the <%= vs. <%- in the templates. The former will embed the variable as HTML, while the latter will escape it (making sure HTML won't execute, turning it into displayable text).

    This is a very important distinction, because mixing these up can lead to security problems or page breaks.

    In this change, tip represents text strings to display to the user, which may be localized (using gettext), and the template is embedding as HTML.

    Now imagine if a translation of one of those strings resulted in a < somewhere in the text, just due to some aspect of that language. This would cause a breakage, since suddenly this template would be trying to render something that looks almost like a tag. Kaboom.

    Another scenario: Someone makes and distributes a malicious localization that translates one of these strings by putting a malicious <script> tag somewhere inside. That'd be hard to notice.

    Because of this, we generally want to err on the side of using <%- instead of <%= in templates, unless we know 100% for sure that we're feeding in some (safe) HTML.

  6. This line looks to be too long. It should wrap at < 80 characters.

  7. How about: "Click a line number to comment on that line. To create a multi-line comment, click-and-drag down multiple line numbers."

  8. I don't feel this tip is very useful as-is. It doesn't say what this actually does, just that you can click on it.

  9. This should use one-space indentation.

  10. Some things to note:

    1. The <p> tag should be on its own line, with content appearing on subsequent lines, indented relative to the <p> (1 space).
    2. This should use more specific class names. Now, the revision stuff predates our modern CSS component standard, but you can see above that we have a set of "revision-selector" classes. I'd like to see you expand this with a revision-selector-help class for the <p>, and make the icon a part of that class (you'd set it on ::before -- look at the CSS for rb-c-alert).
    3. The explanation text must be localized. You'll need to pass localized text as a template parameter, and embed it here, like you did in the other template.
  11. Text like "You could ..." isn't really ideal for UI text. This also isn't saying how you could select specific commits.

    Something better would be:

    Drag the handles to select the uploaded diff revision or range of revisions to view. Selecting two revisions will show the differences between the two."
    
  12. 
      
xiaole2
Review request changed

Change Summary:

  1. change one-space indentation.
  2. edit/update help text content more accurate. (Would like to have more formal languages suggestion)
  3. make style consistent by applying rb-c-alert -is-info, rb-c-alert__content, and rb-c-alert__heading.

Description:

~  

Add inline help text for revision selector, how to make comments, and the expander/collapse button.

~   The inline help text for revision selector will show up when there is a revision selector(which means you need to have more than one version).

  ~

Add inline help text for revision selector, how to make comments, and the expander/collapse button in View Diff page

  ~ The inline help text for revision selector will show up when there is a revision selector(which means you need to have more than one version).
  + The inline help text for expander/collapse buttons will switch based on current button status.

Commits:

Summary Author
-
view diff page inline text
XiaoleZ
+
view diff page inline text
XiaoleZ
+
1. change one-space indentation. 2. edit/update help text more accurate. 3. make style consistent
XiaoleZ
+
remove trailing space
XiaoleZ

Diff:

Revision 2 (+96 -28)

Show changes

Removed Files:

Added Files:

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...