-
-
This is really nit-picky, but I feel like there could be a little more space between the input for testing and the legend. Compared to the rest of the spacing it seems a little cramped to me
-
I'm not sure why this is called required-field-legacy. What do you mean by legacy? Is this a convention I don't know about?
Added * flag to required review request fields
Review Request #2881 — Created Feb. 13, 2012 and submitted
Added * flag to required review request fields. The rest of this review is in review 2889 under djblets.
Visually inspected on Chrome 14, on ubuntu 11.04.
| Description | From | Last Updated |
|---|---|---|
|
There should be more of a margin above this. Maybe another 0.5em. |
 chipx86
|
|
|
You're right, that is a problem. So for this particular case, I'd just not show the "*" for these fields … |
chipx86
|
|
|
I'd just call this "required". |
chipx86
|
|
|
There's no point in showing the asterisk to users unless they can modify the review request. Perhaps instead, have the … |
chipx86
|
|
|
This should also only be shown when you can edit. |
chipx86
|
|
|
Should the {% if %} and {% endif %} blocks really be indented? I've seen some of our other template … |
 mike_conley
|
SM
-
Hmm, so the asterisk works in theory, but I'm not sure I love the placement in practice. I wonder if we can play with this a bit more. Partly, the asterisk is too small. It's nothing but a dot in the screenshot. There's a different route I'm thinking of, that I'd love for you to try. That would be placing it essentially by the pencil icon, a bit larger. This ties it to the thing you click to edit, rather than being on the other side of the label.
-
-
There's no point in showing the asterisk to users unless they can modify the review request. Perhaps instead, have the function that sets up the editor take an argument for whether or not it's required. It can then place the asterisk when it adds the pencil (ensuring it'll only ever be seen if the user can edit).
-
ME
- Change Summary:
-
Required ield instructions are now only visible to editor. Required field flags are now inserted dynamically with edit icon. Fixed typo. Increased spacing for the legend.
- Description:
-
~ Added * flag to required review request fields.
~ Added "* Required Field" legend. ~ Added * flag to required review request fields.
~ + Concerns:
+ + The flag is shown for both the "Groups" and "People" fields, indicating both fields are required, when only one is required.
- Diff:
-
Revision 2 (+26 -5)
- Screenshots:
-
Required Fields *Required Fields *
ME
- Change Summary:
-
Added review 2889 reference.
- Description:
-
Added * flag to required review request fields.
+ The rest of this review is in review 2889 under djblets.
+ Concerns:
The flag is shown for both the "Groups" and "People" fields, indicating both fields are required, when only one is required.
ME
- Change Summary:
-
Moved required flag from "Groups" and "People" to "Reviewers". Increased padding on required flag legend.
- Description:
-
Added * flag to required review request fields.
The rest of this review is in review 2889 under djblets.
- - Concerns:
- - The flag is shown for both the "Groups" and "People" fields, indicating both fields are required, when only one is required.
- Diff:
-
Revision 3 (+30 -4)
- Screenshots:
-
Required Fields *User with permission viewUser without permission view
-
Looks pretty good - just one question.
-
Should the {% if %} and {% endif %} blocks really be indented? I've seen some of our other template code where we use: {% if x %} <span class="required">*</span> {% if y %} <span class="other-required">*</span> {% endif %} {% endif %} So perhaps the if/endif should not be indented at all?