[WIP] Inline help text for view diff page
Review Request #10910 — Created Feb. 21, 2020 and discarded
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 |
---|---|---|
a96825aaea1fe8ac37a1d1adf8e901b9c84123ac | XiaoleZ | |
ee27df59d993760478d0b781742ea40eb3796c54 | XiaoleZ | |
3914e88977f7b96fc7397312b49dc52a262bf1ce | XiaoleZ |
Description | From | Last Updated |
---|---|---|
I would swap the order, but any help text above the operations. |
chipx86 | |
We use 1-space indentation in HTML. |
chipx86 | |
I thought I had written this already on your change for rb-c-tip, but I didn't. Oops! I know I started … |
chipx86 | |
You may have noticed the <%= vs. <%- in the templates. The former will embed the variable as HTML, while … |
chipx86 | |
This line looks to be too long. It should wrap at < 80 characters. |
chipx86 | |
How about: "Click a line number to comment on that line. To create a multi-line comment, click-and-drag down multiple line … |
chipx86 | |
I don't feel this tip is very useful as-is. It doesn't say what this actually does, just that you can … |
chipx86 | |
This should use one-space indentation. |
chipx86 | |
Some things to note: The <p> tag should be on its own line, with content appearing on subsequent lines, indented … |
chipx86 | |
Text like "You could ..." isn't really ideal for UI text. This also isn't saying how you could select specific … |
chipx86 |
- Change Summary:
-
leave comments for reviewer
-
Screen Shot 2020-02-21 at 10.38.19 PM.png: Screen Shot 2020-02-21 at 10.38.19 PM.pngrevision selectorScreen Shot 2020-02-21 at 10.40.21 PM.png: Screen Shot 2020-02-21 at 10.40.21 PM.pngcomments, 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?
-
-
-
-
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.
-
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. -
-
How about: "Click a line number to comment on that line. To create a multi-line comment, click-and-drag down multiple line numbers."
-
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.
-
-
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
-
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:
-
~ 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 ID Author a96825aaea1fe8ac37a1d1adf8e901b9c84123ac XiaoleZ a96825aaea1fe8ac37a1d1adf8e901b9c84123ac XiaoleZ ee27df59d993760478d0b781742ea40eb3796c54 XiaoleZ 3914e88977f7b96fc7397312b49dc52a262bf1ce XiaoleZ - Diff:
-
Revision 2 (+96 -28)
- Removed Files:
- Added Files: