[Freeipa-devel] [PATCH] 295 Fixed inconsistent required/optional attributes.

[Endi Sukma Dewata]

On 10/21/2011 6:40 AM, Petr Vobornik wrote:
> 1) Wouldn't be better if the asterisk has different color than the
> label? Visually I don't like it that much and I think it can be
> overlook. Attaching a proposition. I used green IPAish color because red
> usually means error.
>
> Code from browser how it was done:
>
> <td class="section-cell-label" title="First name"><label
> name="givenname" class="field-label">First name:</label><span
> class="required" style="
> float: right;
> font-weight: bold;
> color: #319016;
> font-size: 20px;
> " title="required">*</span></td>
>
> (style should be moved to css file)
>
>
> <div style="line-height: 25px;"><span class="required" style="
> font-weight: bold;
> color: #319016;
> font-size: 20px;
> vertical-align: middle;
> ">*</span> required</div>
>
> It may vary on the section type.

Fixed. I couldn't use the exact style above because it looks a bit weird 
in the details page since the labels are right aligned. I use red for 
now because it can also mean 'important', but we can adjust this again 
later.

> 2) When rendering label, we should also obtain field input's id (if
> possible) for 'for' attribute of <label>. This can be done separately.

I'd rather avoid using ID in a dynamic app like this. Adding ID into the 
fields is possible, but we'd have to do it very carefully to avoid 
conflicts.

> 3) Should we create some common pure html widgets with certain
> semantics?

We can, but as discussed elsewhere, we have to fix the current 
assumption first: each widget correspond to an attribute, it currently 
has properties and methods (e.g. metadata, load(), save()) that are only 
valid on attributes.

> IE asterisk shouldn't be directly concatenated with label
> text. It is used on more than one place which may cause maintenance issues.
>
> IPA.form(or some other name).required_indicator = function() {
> return '*'
> };
>
> in this case this seems unnecessary. But if the required indicator was
> like in 1) it will be useful.

Fixed.

> Summary:
> All 3 points are nice to have. If you think is not necessary then ACK.
>
> This patch is also fixing https://fedorahosted.org/freeipa/ticket/1973 .

Noted in the new patch.

-- 
Endi S. Dewata
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-edewata-0295-2-Fixed-inconsistent-required-optional-attributes.patch
Type: text/x-patch
Size: 34817 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111024/31b71f58/attachment.bin>