[Ovirt-devel] [PATCH server] Version 1 of Revamped Tree Navigation. Fixed css urls
Mohammed Morsi
mmorsi at redhat.com
Wed Oct 8 17:19:39 UTC 2008
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 ;-)
Jason Guiditta wrote:
> The new javascript tree widget contains the following features/changes from previous implementation:
>
> * The html for the list is dynamically generated using a javascript template system. This will allow us to plug in different layouts per tree as the widget matures.
> * Updates to the tree are now incremental, rather than a full rip and replace as earlier. We have 2 states we currently look for - 'new' and 'changed'. The first generates new html and appends it to the DOM, the second just does a replacement of the content of existing nodes.
> * Vastly simplified the markup and css.
> * Added calls where appropriate to refresh the tree before next planned call (for instance, if you add a new hardware pool).
>
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.
> * Added slide effect when opening and closing a node of the tree.
> * Clicking the plus/minus opens/close the node only, does not load main content area.
> * Clicking anywhere to the right of that on a given node will load content area.
>
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.
> * Added interim icons for 'smartpool' and 'add smartpool'
>
> Note that aside from the nav area, this should not impact the existing trees which have not been converted yet (all popups that have one), as this is a completely separate codebase with it's own js and css files.
>
> Related, but not technically part of the tree, I added a choose_layout method to allow testing of javascript components as we are building them to help eliminate possible side effects from other code. When not in a production environment, you can pass in ?component_layout=[name] where [name] is the name of a shell rhtml file you have put in views/layouts/components. As our UI is growing increasingly complex, I think this will be a very useful way to facilitate building components.
>
>
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.
[snip]
> diff --git a/src/public/javascripts/smart_nav_test_data.js b/src/public/javascripts/smart_nav_test_data.js
> new file mode 100644
> index 0000000..43e7dbc
> --- /dev/null
> +++ b/src/public/javascripts/smart_nav_test_data.js
> @@ -0,0 +1,151 @@
> +var pools3 = {
> + "deleted" : {},
> + "pools" :[
> + { "name": "default",
> + "text": "default",
> + "children":
> + [{ "name": "Engineering",
> + "text": "Engineering",
> + "children":
> + [{ "name": "Development",
> + "text": "Development",
> + "children":
> + [{ "name": "Project X",
> + "text": "Project X",
> + "id": 19,
> + "type": "VmResourcePool"},
> + { "name": "Project Y",
> + "text": "Project Y",
> + "id": 20,
> + "type": "VmResourcePool"}],
> + "id": 9,
> + "type": "HardwarePool"},
> + { "name": "QA",
> + "text": "QA",
> + "children":
> + [{ "name": "Bob's Team",
> + "text": "Bob's Team",
> + "children":
> + [{ "name": "Bob's VMs",
> + "text": "Bob's VMs",
> + "id": 21,
> + "type": "VmResourcePool"}],
> + "id": 17,
> + "type": "HardwarePool"},
> + { "name": "Jim's Team",
> + "text": "Jim's Team",
> + "children":
> + [{ "name": "Jim's VMs",
> + "text": "Jim's VMs",
> + "id": 22,
> + "type": "VmResourcePool"}],
> + "id": 18,
> + "type": "HardwarePool"},
> + { "name": "Sally's Team",
> + "text": "Sally's Team",
> + "children":
> + [{ "name": "Sally's VMs",
> + "text": "Sally's VMs",
> + "id": 33,
> + "type": "VmResourcePool"}],
> + "id": 32,
> + "type": "HardwarePool"}],
> + "id": 10,
> + "type": "HardwarePool"},
> + { "name": "Stage",
> + "text": "Stage",
> + "children":
> + [{ "name": "stage1",
> + "text": "stage1",
> + "id": 45,
> + "type": "HardwarePool"},
> + { "name": "stage2",
> + "text": "stage2",
> + "id": 46,
> + "type": "HardwarePool"}],
> + "id": 44,
> + "type": "HardwarePool"}],
> + "id": 5,
> + "type": "HardwarePool"},
> + { "name": "Finance",
> + "text": "Finance",
> + "children":
> + [{ "name": "Payroll",
> + "text": "Payroll",
> + "children":
> + [{ "name": "Payroll VMs",
> + "text": "Payroll VMs",
> + "id": 23,
> + "type": "VmResourcePool"}],
> + "id": 11,
> + "type": "HardwarePool"},
> + { "name": "Accts. Receivable",
> + "text": "Accts. Receivable",
> + "children":
> + [{ "name": "our VMs",
> + "text": "our VMs",
> + "id": 24,
> + "type": "VmResourcePool"}],
> + "id": 12,
> + "type": "HardwarePool"}],
> + "id": 6,
> + "type": "HardwarePool"},
> + { "name": "HR",
> + "text": "HR",
> + "children":
> + [{ "name": "Hiring Team",
> + "text": "Hiring Team",
> + "id": 13,
> + "type": "HardwarePool"},
> + { "name": "Benefits",
> + "text": "Benefits",
> + "id": 14,
> + "type": "HardwarePool"}],
> + "id": 7,
> + "type": "HardwarePool"},
> + { "name": "External (DMZ)",
> + "text": "External (DMZ)",
> + "children":
> + [{ "name": "VMs",
> + "text": "VMs",
> + "id": 25,
> + "type": "VmResourcePool"},
> + { "name": "DB Cluster",
> + "text": "DB Cluster",
> + "children":
> + [{ "name": "VMs",
> + "text": "VMs",
> + "id": 27,
> + "type": "VmResourcePool"}],
> + "id": 26,
> + "type": "HardwarePool"}],
> + "id": 8,
> + "type": "HardwarePool"}],
> + "id": 1,
> + "type": "HardwarePool"}],
> +"smart_pools":[{ "name": "ovirtadmin",
> + "text": "ovirtadmin",
> + "children":
> + [{ "name": "not so smart",
> + "text": "not so smart",
> + "id": 39,
> + "type": "SmartPool"},
> + { "name": "a little smarter",
> + "text": "a little smarter",
> + "id": 40,
> + "type": "SmartPool"},
> + { "name": "arrrrr",
> + "text": "arrrrr",
> + "id": 41,
> + "type": "SmartPool"},
> + { "name": "huh?",
> + "text": "huh?",
> + "id": 42,
> + "type": "SmartPool"},
> + { "name": "booya",
> + "text": "booya",
> + "id": 43,
> + "type": "SmartPool"}],
> + "id": 37,
> + "type": "DirectoryPool"}]
> +}
>
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?
> \ 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! ;-)
> 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.
Just to reiterate, even with the above issues, the new tree looks good
(perhaps the last padding issue can be tweaked before pushed) so ACK.
-Mo
More information about the ovirt-devel
mailing list