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
