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

[Petr Vobornik]

On 10/24/2011 10:43 PM, Endi Sukma Dewata wrote:
> 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.
>

ACK

Also proposing minor visual enhancement (see attached patch).

-- 
Petr Vobornik

-------------- next part --------------
A non-text attachment was scrubbed...
Name: wip-freeipa-pvoborni-0005-Minor-visual-enhancement-of-required-indicator.patch
Type: text/x-patch
Size: 1624 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111025/fe14ad9a/attachment.bin>