[Ovirt-devel] [PATCH server] Version 1 of Revamped Tree Navigation. Fixed css urls

[Jason Guiditta]

On Wed, 2008-10-08 at 13:19 -0400, Mohammed Morsi wrote:
> ACK as I tested it out and it seems to work on my system, and did a code
> review. There are a few caveats as described below, but no blockers that
> would prevent this from being pushed. (I can add these issues to
> bugzilla if desired / required). One general note, there are alot of
> deletions / additions due to whitespace removal, and while I and I'm
> sure everyone else appreciates the effort, it did make this patch review
> a bit more complicated. rabble rabble rabble ;-)

On the whitespace removal, there really are only a few I think, as a lot
of this is legitimate reshuffling and moving existing code around with
slight revamps.  Also, I am using the git precommit hook, so I always
fix any whitespace errors that come up.  We can all discuss more in irc,
but I believe we should just fix these as we come across them, as new
ones should not be creeping in with the current setup.  I doubt there
are many left by now anyway, so it may be almost a moot point.

> Not sure what the root cause is but I'm not seeing this 'immediate'
> refresh when I add a new smart, hardware, or virtual machine pool, eg
> the item doesn't appear until the next scheduled tree refresh.
> 
I think this was mentioned in irc, but it is likely browser caching.  I
ran into the same thing on first deploy to an appliance
> 

> Once again, not sure if this is an issue in your patch or something on
> my local setup but this seems to stop after a certain point. eg. I can
> click a bit of whitespace to the right of the node and have the content
> area loaded correctly, but continuing further right (still in the side
> bar though) the area becomes unclickable. I am running Firefox maximized
> to a 1280x1024 screen.
> 
Not sure on this, could be the bit of padding I put to decrease overlap
of the container though.
> 

> No existing issue with this bit, but I'm just wondering if this could
> potentially result in any security issues as the client can now control
> which layout is returned / rendered with which controller / action. Not
> that we should be doing this in the first place, but should we ever have
> a simple security mechanism in javascript (or even javascript form
> validation without a server side correspondence), the client can
> potentially invoke an action with a blank layout / template to get
> around those mechanisms. Perhaps this is a far-fetched scenario, but I
> would just like to raise the issue for thought at this point.

Well, I have taken the security consideration into account by only
allowing the component layout if you are not in a production
environment.  If you think we need more than that, I am open to adding
more security.

> >   
> Is there any way we can add this to the tests/ directory and/or
> integrate this into out test suite so that the tree is tested every time
> autobuild tests the wui? Perhaps automatically loading this tree data
> when the environment is set to 'test'? Perhaps adding a selenium test?
> 
We could, but I think our regular wui selenium tests will work fine as
is.  This is more for development-level testing, ie trying to figure out
the correct jq selectors to use, or how to properly manipulate the dom.
That said, if no one else thinks it is useful, we can always kill it and
I'll just do it locally.  I do need it for my own work though and
running it in a test is not really going to help my use case.  Another
option could be to do a tests directory somewhere strictly for
javascript ui components that includes a test page - this would
accomplish the same thing I am going for here, and not even require
mongrel be running to do tests (hmm, though what about ajax calls..?)
> 
> > \ No newline at end of file
> > diff --git a/src/public/javascripts/trimpath-template-1.0.38.js b/src/public/javascripts/trimpath-template-1.0.38.js
> >   
> Ahhhh! Another javascript library, run for the hills! ;-)
/me hits the pavement....
> 
> 
> > diff --git a/src/public/stylesheets/ovirt-tree/tree.css b/src/public/stylesheets/ovirt-tree/tree.css
> > new file mode 100644
> > index 0000000..5fefb90
> > --- /dev/null
> > +++ b/src/public/stylesheets/ovirt-tree/tree.css
> > @@ -0,0 +1,83 @@
> > +#nav_tree {
> > +   padding: 20px;
> > +}
> >   
> Are you sure you want this here, removing it seems to make the tree look
> a bit nicer. (eg I'm seeing too much whitespace between the 'Dashboard'
> and the 'Default' pool, can send a screenshot if you want.

Sure, we can tweak this as necessary, though I think there is a chance
what you saw was related to the caching issue mentioned above, so I am
leaving it for the moment.
Thanks for the feedback,

-j