[WIP] Inline help text for view diff page

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

Information

Review Board
master

Reviewers

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 ID Author
view diff page inline text
a96825aaea1fe8ac37a1d1adf8e901b9c84123ac XiaoleZ
1. change one-space indentation. 2. edit/update help text more accurate. 3. make style consistent
ee27df59d993760478d0b781742ea40eb3796c54 XiaoleZ
remove trailing space
3914e88977f7b96fc7397312b49dc52a262bf1ce XiaoleZ

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

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

  3. Show all issues

    We use 1-space indentation in HTML.

  4. Show all issues

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

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

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

  7. Show all issues

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

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

    This should use one-space indentation.

  10. Show all issues

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

    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
david
Review request changed
Status:
Discarded