Re: [Freeipa-devel] [PATCH] 647-651 [webui] Make utility section of navigation extensible
On 23.6.2014 18:35, Endi Sukma Dewata wrote: On 6/23/2014 8:15 AM, Petr Vobornik wrote: 1. I'm not sure if we really need a HashCreator. Ideally the router should map a hash to a page. Links to another page can be hardcoded too (and substitute the parameters). The main purpose of a hash creator is to update hash when a facet state changes. This change can be initiated from the facet itself, e.g. when searching in a search facet. Removing the hash creator would make facets dependent on navigation specifics. I was thinking if the facet itself is changing the state, it will only change the query part of the URL. The base facet URL itself is assigned by the router. I think we used to put all UI states in the URL so that's why we needed a hash creator. My thinking was that the facet shouldn't be even aware of the hash or routing. I would agree if it was related only to links. But even then it would be better to have a method which would create the hash (for the one which shares the same pattern) to reduce code reuse. In any case, it's possible to hardcode hash parts in links if one needs to. It's more about removing dependencies between pages. For example, to link from one page to another we use something like this: navigation.show_entity(entity.name, facetname, [pkey]); This means we're explicitly handling the link. And this also puts additional restrictions such as the target page must be an entity page which has one level sub page called 'facetname' that takes one primary key. If the target page is not an entity page we'd have to use a different method. I think it would be cleaner if the link can be defined in a (future) template like this: a href=#/{entity}/{facet}/{pkey} How is this different? You still have to pass the entity, facet name and pkey. You just moved the logic form hash creator to the template. And I can bet that doing something more difficult there will be a pain. Also it doesn't work for redirection initiated by other means then a link, e.g., when something gets deleted. If one wants to create a link in a template he could use some link helper which would internally call the hash creator. Note that the template is used to generate the links only. When the user clicks the link we rely on the browser, or other routing libraries, to handle it so less code to maintain and it puts the UI workflow in the hands of UI designers. Keeping facet and widget internals out of direct hash modification allows to embed the whole app or just some parts of it into other app. Or we can have two different or same facets displayed next to each other. Atm this is not an argument since we do not make it with this use case in mind. The same applies for code modifications by designers. 2. Ideally there should be no entity-specific navigation code. All routing should be handled in a generic way. This probably depends on the entity-facet separation that we discussed previously. So routes like this: /e/:entity/:facet/:pkeys/*args should be replaced by individual routes for each entity: /user/:facet/:pkeys/*args /group/:facet/:pkeys/*args Yes, but IMHO the hash prefix is a detail. It would be more important if it would be a REST API where it's a core aspect. Right, a REST URL probably would look like: /ipa/ui/users/{facet}/{pkey}?{query} And ideally if you open a REST URL in a browser you should get a UI. So anything that brings the UI closer to this would be great. 4. The args/query in the navigation paths should be separated by a more standard delimiter ? instead of //. For example: /ipa/ui/#/e/user/search//filter=test should be replaced with: /ipa/ui/#/e/user/search?filter=test note that // is a result of /:pkeys/, where :pkeys is '' Therefore a simple replacement would lead to /ipa/ui/#/e/user/search/?filter=test Please file a ticket, if you think it's important. I was originally thinking this to be a way to mimic the REST URL, but now I'm not sure if it would interfere with URL parsing because in this case the ? will be part of the URL hash, not URL query, so maybe we should just leave it for now. Lets say that we would implement #2 and #4. Would we also want to keep the old routes for backwards compatibility? (main purpose of hashes is to make the page bookmarkable). IMHO people don't care about #2 and #4 much, but they care about broken bookmarks. The hashes are probably more useful to make the browser's Back button functional. Bookmarking is probably a secondary benefit, but are there pages in the UI that people would likely to bookmark? An admin probably would just bookmark the main URL, not a particular page in the UI. Backward compatibility of routing is nice, but I don't think it's a hard requirement. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 647-651 [webui] Make utility section of navigation extensible
On 20.6.2014 18:18, Endi Sukma Dewata wrote: On 6/18/2014 6:11 AM, Petr Vobornik wrote: 1. As discussed on IRC, the plugin is causing an error due to missing extend.js. This needs to be fixed. Fixed 4. I agree that the facet shouldn't define the hash. The hash should be part of the plugin declaration. Ideally, facet should be router agnostic. We need a mechanism of mapping facet's state to hash and vice versa. Originally I did not want to do it now because it requires bigger refactoring of a router. While I was designing it, I ended up with a patch - attached. It's a replacement for original patch #649. Patch #651 and example plugin are updated accordingly. Basically I refactored router's `navigate_to_*`, `_create_*_hash` and `*_route_handler` methods into separate classes which are registered and mapped to facets. All in navigation.routing module. Btw, can you think of some better name(s)/placement for the module? ACK. Some comments below, but I think they can be addressed later. pushed to master: * c6c7dfeefbb8b94f102aef6a04d828530e3ccb0f webui: fix excessive registration of state change event listeners * 27836cba9d865b1c912a65d0cd04562194f9e93f webui: support standalone facets in navigation module * 86898065b5e1d60168e2daff050853729b34f1ce webui: generic routing * 6f5e80b0cec57a89a68f2935b5fe01d919b11031 webui: add parent link to widgets in ContainerMixin * 6e43d01266f105cdf3cf27a1dbb87ac80da4a06d webui: plugin API 1. I'm not sure if we really need a HashCreator. Ideally the router should map a hash to a page. Links to another page can be hardcoded too (and substitute the parameters). The main purpose of a hash creator is to update hash when a facet state changes. This change can be initiated from the facet itself, e.g. when searching in a search facet. Removing the hash creator would make facets dependent on navigation specifics. I would agree if it was related only to links. But even then it would be better to have a method which would create the hash (for the one which shares the same pattern) to reduce code reuse. In any case, it's possible to hardcode hash parts in links if one needs to. 2. Ideally there should be no entity-specific navigation code. All routing should be handled in a generic way. This probably depends on the entity-facet separation that we discussed previously. So routes like this: /e/:entity/:facet/:pkeys/*args should be replaced by individual routes for each entity: /user/:facet/:pkeys/*args /group/:facet/:pkeys/*args Yes, but IMHO the hash prefix is a detail. It would be more important if it would be a REST API where it's a core aspect. Probably the entities should be written like plugins. They are, in a way... I'm toying with the idea of moving them (user.js, group.js, ...) to plugins dir. 3. If we fix #1 and #2, is the routing module still necessary? The routing object probably should have been a router, but are you concerned about conflicting with Dojo's router and the existing Router module? Our 'Router' is just an encapsulation over the Dojo's router. It's true that with this refactoring, the encapsulation became so thin that the logic could be moved to 'routing' and 'routing' could be then renamed to 'router'. There would be no conflict since the modules are in different packages. If we want to do it, we should do it in 4.0 (ASAP). 4. The args/query in the navigation paths should be separated by a more standard delimiter ? instead of //. For example: /ipa/ui/#/e/user/search//filter=test should be replaced with: /ipa/ui/#/e/user/search?filter=test note that // is a result of /:pkeys/, where :pkeys is '' Therefore a simple replacement would lead to /ipa/ui/#/e/user/search/?filter=test Please file a ticket, if you think it's important. Lets say that we would implement #2 and #4. Would we also want to keep the old routes for backwards compatibility? (main purpose of hashes is to make the page bookmarkable). IMHO people don't care about #2 and #4 much, but they care about broken bookmarks. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 647-651 [webui] Make utility section of navigation extensible
On 6/23/2014 8:15 AM, Petr Vobornik wrote: 1. I'm not sure if we really need a HashCreator. Ideally the router should map a hash to a page. Links to another page can be hardcoded too (and substitute the parameters). The main purpose of a hash creator is to update hash when a facet state changes. This change can be initiated from the facet itself, e.g. when searching in a search facet. Removing the hash creator would make facets dependent on navigation specifics. I was thinking if the facet itself is changing the state, it will only change the query part of the URL. The base facet URL itself is assigned by the router. I think we used to put all UI states in the URL so that's why we needed a hash creator. I would agree if it was related only to links. But even then it would be better to have a method which would create the hash (for the one which shares the same pattern) to reduce code reuse. In any case, it's possible to hardcode hash parts in links if one needs to. It's more about removing dependencies between pages. For example, to link from one page to another we use something like this: navigation.show_entity(entity.name, facetname, [pkey]); This means we're explicitly handling the link. And this also puts additional restrictions such as the target page must be an entity page which has one level sub page called 'facetname' that takes one primary key. If the target page is not an entity page we'd have to use a different method. I think it would be cleaner if the link can be defined in a (future) template like this: a href=#/{entity}/{facet}/{pkey} Note that the template is used to generate the links only. When the user clicks the link we rely on the browser, or other routing libraries, to handle it so less code to maintain and it puts the UI workflow in the hands of UI designers. 2. Ideally there should be no entity-specific navigation code. All routing should be handled in a generic way. This probably depends on the entity-facet separation that we discussed previously. So routes like this: /e/:entity/:facet/:pkeys/*args should be replaced by individual routes for each entity: /user/:facet/:pkeys/*args /group/:facet/:pkeys/*args Yes, but IMHO the hash prefix is a detail. It would be more important if it would be a REST API where it's a core aspect. Right, a REST URL probably would look like: /ipa/ui/users/{facet}/{pkey}?{query} And ideally if you open a REST URL in a browser you should get a UI. So anything that brings the UI closer to this would be great. 4. The args/query in the navigation paths should be separated by a more standard delimiter ? instead of //. For example: /ipa/ui/#/e/user/search//filter=test should be replaced with: /ipa/ui/#/e/user/search?filter=test note that // is a result of /:pkeys/, where :pkeys is '' Therefore a simple replacement would lead to /ipa/ui/#/e/user/search/?filter=test Please file a ticket, if you think it's important. I was originally thinking this to be a way to mimic the REST URL, but now I'm not sure if it would interfere with URL parsing because in this case the ? will be part of the URL hash, not URL query, so maybe we should just leave it for now. Lets say that we would implement #2 and #4. Would we also want to keep the old routes for backwards compatibility? (main purpose of hashes is to make the page bookmarkable). IMHO people don't care about #2 and #4 much, but they care about broken bookmarks. The hashes are probably more useful to make the browser's Back button functional. Bookmarking is probably a secondary benefit, but are there pages in the UI that people would likely to bookmark? An admin probably would just bookmark the main URL, not a particular page in the UI. Backward compatibility of routing is nice, but I don't think it's a hard requirement. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 647-651 [webui] Make utility section of navigation extensible
On 6/18/2014 6:11 AM, Petr Vobornik wrote: 1. As discussed on IRC, the plugin is causing an error due to missing extend.js. This needs to be fixed. Fixed 4. I agree that the facet shouldn't define the hash. The hash should be part of the plugin declaration. Ideally, facet should be router agnostic. We need a mechanism of mapping facet's state to hash and vice versa. Originally I did not want to do it now because it requires bigger refactoring of a router. While I was designing it, I ended up with a patch - attached. It's a replacement for original patch #649. Patch #651 and example plugin are updated accordingly. Basically I refactored router's `navigate_to_*`, `_create_*_hash` and `*_route_handler` methods into separate classes which are registered and mapped to facets. All in navigation.routing module. Btw, can you think of some better name(s)/placement for the module? ACK. Some comments below, but I think they can be addressed later. 1. I'm not sure if we really need a HashCreator. Ideally the router should map a hash to a page. Links to another page can be hardcoded too (and substitute the parameters). 2. Ideally there should be no entity-specific navigation code. All routing should be handled in a generic way. This probably depends on the entity-facet separation that we discussed previously. So routes like this: /e/:entity/:facet/:pkeys/*args should be replaced by individual routes for each entity: /user/:facet/:pkeys/*args /group/:facet/:pkeys/*args Probably the entities should be written like plugins. 3. If we fix #1 and #2, is the routing module still necessary? The routing object probably should have been a router, but are you concerned about conflicting with Dojo's router and the existing Router module? 4. The args/query in the navigation paths should be separated by a more standard delimiter ? instead of //. For example: /ipa/ui/#/e/user/search//filter=test should be replaced with: /ipa/ui/#/e/user/search?filter=test -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 647-651 [webui] Make utility section of navigation extensible
On 5/27/2014 6:15 AM, Petr Vobornik wrote: This is just a first draft of implementation of https://fedorahosted.org/freeipa/ticket/4345 It introduces a `freeipa/extend` module which should serve as a more stable API for Web UI plugins. I think it requires further discussion - what to have there, the level of abstraction... Other patches fixes navigation/router to support custom URL path patterns. The usage of facet.create_hash(router) method in patch #649 is questionable. The WIP patch contains an example of a plugin which uses this functionality. Some comments: 1. As discussed on IRC, the plugin is causing an error due to missing extend.js. This needs to be fixed. 2. Also discussed, a base widget class for StateWidget might help providing a structure and simplifying the plugin code by defining the default widget behavior (e.g. event registration). 3. Related to inheritance, instead of conditionally calling a method like this: if (facet.create_hash) return facet.create_hash(router); // otherwise, do the default behavior if we use inheritance it could be simplified like this: return facet.create_hash(router); So the method will do the default behavior as defined in the base class unless it's overridden by the subclass. 4. I agree that the facet shouldn't define the hash. The hash should be part of the plugin declaration. 5. Also discussed about moving away from declarative entity/facet definitions into template-based framework. It could help reduce the code complexity. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 647-651 [webui] Make utility section of navigation extensible
This is just a first draft of implementation of https://fedorahosted.org/freeipa/ticket/4345 It introduces a `freeipa/extend` module which should serve as a more stable API for Web UI plugins. I think it requires further discussion - what to have there, the level of abstraction... Other patches fixes navigation/router to support custom URL path patterns. The usage of facet.create_hash(router) method in patch #649 is questionable. The WIP patch contains an example of a plugin which uses this functionality. -- Petr Vobornik From c7582a3fe8d26b51028e1da24642f2456ae0f7f4 Mon Sep 17 00:00:00 2001 From: Petr Vobornik pvobo...@redhat.com Date: Fri, 16 May 2014 18:32:17 +0200 Subject: [PATCH] webui: plugin API new `extend` module should serve as a stable API for plugin authors. It should expose the most commonly used global calls. https://fedorahosted.org/freeipa/ticket/4345 --- install/ui/src/freeipa/extend.js | 76 1 file changed, 76 insertions(+) create mode 100644 install/ui/src/freeipa/extend.js diff --git a/install/ui/src/freeipa/extend.js b/install/ui/src/freeipa/extend.js new file mode 100644 index ..e081aaad60a448d213abe57fc19836d6439def65 --- /dev/null +++ b/install/ui/src/freeipa/extend.js @@ -0,0 +1,76 @@ +/* Authors: + *Petr Vobornik pvobo...@redhat.com + * + * Copyright (C) 2014 Red Hat + * see file 'COPYING' for use and warranty information + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. +*/ + +define([ +'dojo/_base/lang', +'./jquery', +'./phases', +'./app_container', +'exports' +],function(lang, $, phases, app, extend) { + +/** + * Extension interface + * + * This class provides interface for plugins and tries to hide underlying functionality + * + * @class extend + * @singleton + */ +lang.mixin(extend, { +/** + * Adds element to utility section + * + * This method doesn't do any correction. Expended root node type to add is + * by default li. + * + * Preferred phase: any after `init` + * + * @param {HTMLElement|jQuery} element Element to add to utility section + * @return {HTMLElement} Utility node + */ +add_menu_utility: function(element) { + +// Should we check if we are in good stage or atleast report that app doesn't exist yet? + +var $utility = $(app.app.app_widget.nav_util_tool_node); +$utility.prepend(element); +return $utility.eq(0); +}, + +/** + * Add route to router + * + * Handle should expect an `event` param. Check `dojo/router` documentation + * for more details + * + * @param {String|RegExp} route A string or regular expression that should + * be used for matching hash changes. + * @param {Function} handler A callback function which is executed on + * match. It's context is the router. + */ +add_route: function(route, handler) { +var router = app.app.router; +router.register_route(route, handler); +} +}); + +return extend; +}); \ No newline at end of file -- 1.9.0 From 3c79ff2d2d51ade7d2807ad90f5ddadfb77741ed Mon Sep 17 00:00:00 2001 From: Petr Vobornik pvobo...@redhat.com Date: Fri, 16 May 2014 18:25:04 +0200 Subject: [PATCH] webui: add parent link to widgets in ContainerMixin Standard facets sets `facet` attribute to widgets. This one adds similar, more generic `parent` attribute which should be used for going through the hierarchy up to top. --- install/ui/src/freeipa/widget.js | 1 + install/ui/src/freeipa/widgets/ContainerMixin.js | 2 ++ 2 files changed, 3 insertions(+) diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js index 212fc1c445e05b5c20c2ccfc7ad4beae4fc5c15e..75bacf0c287750ff0e52e7da20ac804d310e8327 100644 --- a/install/ui/src/freeipa/widget.js +++ b/install/ui/src/freeipa/widget.js @@ -5665,6 +5665,7 @@ exp.activity_widget = IPA.activity_widget = function(spec) { exp.pre_op = function(spec, context) { if (context.facet) spec.facet = context.facet; +if (context.parent) spec.parent = context.parent; if (context.entity) spec.entity = context.entity; return spec; }; diff --git a/install/ui/src/freeipa/widgets/ContainerMixin.js