[WIP] Inline help text for view diff page
Review Request #10910 — Created Feb. 21, 2020 and discarded
Information | |
---|---|
xiaole2 | |
Review Board | |
master | |
Reviewers | |
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 |
---|---|
XiaoleZ | |
XiaoleZ | |
XiaoleZ |
Description | From | Last Updated |
---|---|---|
I would swap the order, but any help text above the operations. |
|
|
We use 1-space indentation in HTML. |
|
|
I thought I had written this already on your change for rb-c-tip, but I didn't. Oops! I know I started … |
|
|
You may have noticed the <%= vs. <%- in the templates. The former will embed the variable as HTML, while … |
|
|
This line looks to be too long. It should wrap at < 80 characters. |
|
|
How about: "Click a line number to comment on that line. To create a multi-line comment, click-and-drag down multiple line … |
|
|
I don't feel this tip is very useful as-is. It doesn't say what this actually does, just that you can … |
|
|
This should use one-space indentation. |
|
|
Some things to note: The <p> tag should be on its own line, with content appearing on subsequent lines, indented … |
|
|
Text like "You could ..." isn't really ideal for UI text. This also isn't saying how you could select specific … |
|

Change Summary:
leave comments for reviewer
-
File Captions:
Screen Shot 2020-02-21 at 10.38.19 PM.png: - Screen Shot 2020-02-21 at 10.38.19 PM.png + revision selector Screen Shot 2020-02-21 at 10.40.21 PM.png: - Screen Shot 2020-02-21 at 10.40.21 PM.png + comments, and the expander/collapse button

-
-
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?
-
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1) We use 1-space indentation in HTML.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1) 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 torb-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 userb-c-alert -is-info
here. See the documentation forrb-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.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1) 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 (usinggettext
), 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. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1) This line looks to be too long. It should wrap at < 80 characters.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1) How about: "Click a line number to comment on that line. To create a multi-line comment, click-and-drag down multiple line numbers."
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1) 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.
-
reviewboard/static/rb/js/views/revisionSelectorView.es6.js (Diff revision 1) This should use one-space indentation.
-
reviewboard/static/rb/js/views/revisionSelectorView.es6.js (Diff revision 1) Some things to note:
- The
<p>
tag should be on its own line, with content appearing on subsequent lines, indented relative to the<p>
(1 space). - 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 forrb-c-alert
). - 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.
- The
-
reviewboard/static/rb/js/views/revisionSelectorView.es6.js (Diff revision 1) 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."

Change Summary:
- change one-space indentation.
- edit/update help text content more accurate. (Would like to have more formal languages suggestion)
- make style consistent by applying
rb-c-alert -is-info
,rb-c-alert__content
, andrb-c-alert__heading
.
Description: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||
Diff: |
Revision 2 (+96 -28) |
|||||||||||||||
Removed Files: |
||||||||||||||||
Added Files: |