[Ovirt-devel] [PATCH server] First step of detail pane UI cleanup.

[Mohammed Morsi]

Jason Guiditta wrote:
>>> +<!-- FIXME: for the love of pete, are we really using a list just to insert a separator??
>>> +     This needs to be cleaned up as soon as somone has time.  It should be a horizontal list
>>> +     for the links (tools), with this separator set as a left background on the first item.
>>> +-->
>>>   
>>>       
>> Does it being a list break anything specifically? I think a list is
>> fine, we're not only using it for a separator but we're styling it in
>> the css to appear horizontal / have the same effect you described. We'd
>> need the same css for the divs if we we're to remove the list plus the
>> exception for the first. So not sure if this comment should go in.
>>     
>
> Mo, really there are two things here.  If it was indeed a list of links,
> that would actually be perfect.  The thing is, while this may have been
> the intent, it was not executed that way.  Take a look at any
> content_for :action_links (for example
> views/resources/quick_summary.rhtml) and you will see not a list but a
> bunch of links not contained in li's.  So not only is it presented as a
> bunch of links, they are links in this context:
> <ul>
>   <li>image</li>
>   <a href="">blah</a>
>   <a href="">blah</a>
> </ul>
>
> which is syntactically incorrect, and would fail any validator we threw
> it at.
>   
Ah, hadn't realized that the actual content isn't generated w/ the <li>
tags that would make sense here. Yes that needs to be syntactically
fixed at some point.


> For the second point I am more trying to push the concept of separating
> content from layout, which is really just an extension of mvc and
> separating presentation from business logic, so we should all feel
> pretty comfortable with the idea.  The idea of using an image as a
> border is completely fine.  However, it is also completely unrelated to
> the content, and therefore belongs in the stylesheet rather than the
> rhtml.  In this case if we really wanted to use an image as the
> separator, I would likely add a class along the lines of:
> .first {
>   background: url('../images/list_separator.png') center left no-repeat;
> }
> There may be some additional setting that need to be dropped in to make
> it look completely correct, but I think the idea is clear.  However, as
> it is just a gray line, my inclination would be to just use border-left
> with a matching color.  One less image to maintain, and preferable css
> style-wise.  Anyway, my point here is to improve our markup quality.
> While some think it is unimportant, I think it reflects poorly on our
> attention to detail to not try to do these things the correct way.  I
> would like to see this treated as more craftsmanship than 'just get it
> done'.
>
> -j
>   
Agreed, we should use style where necessary / appropriate. Either way,
by adding the li's to the appropriate detail-pane templates, as we're
still dealing w/ a list, or adding divs, styling would probably most be
suited for the css stylesheet.


>> Other than that, the patch looks good, the details pane output looks
>> just like Jeremy's screenshot.
>>
>>     -Mo
>>     
>
>   

Once again ACK still stands, perhaps with a bit of rewording in the
comment ( for the love of pete ;-) ), but its not a show stopper so you
can go ahead and push it.

    -Mo