Open Stack

Hi Thai,

>From your example, option 1 seems closer to the *current pattern* not
option 2. :) Where the user define a list of action separately from the
table presentation (HTML template) rather than embedding it in the HTML.
And if the user wants to extend it, they just add it to the list of
columns/actions on the table class.

Option 1 seems better to me due to I find it closer to the current pattern.
As long as we can reduce the duplicate code (not having to write 9 files to
create one table), I'm good with that. :)

My main concern is really to polish first the initial table implementation
before folks jump into implementing the tables in all other panels.  So we
can avoid re-work, don't want another cycle of clean-up/refactor. :)

I think we already have 2 angular tables out, should be enough data to
figure out what duplicate code can be abstracted out based from those two
implementation.

-Lin


On Thu, Aug 20, 2015 at 4:36 PM, Doug Fish <the.doug.fish at gmail.com> wrote:

> It appears to me that option 1 would be better prepared to be extensible
> ... That is if a plugin needed to add an action or a column, we could make
> that happen with pattern 1 (possibly after adding in a service) I'm not
> sure how plugins ever add these things with pattern 2.
>
> On Aug 20, 2015, at 1:41 PM, "Thai Q Tran" <tqtran at us.ibm.com> wrote:
>
> Hi Lin,
>
> Let me draw on some examples to help clarify what I mean.
>
> *Option 1:*
>
> table.controller.js
> --------------------
> ctrl.headers = {
>   gettext('column 1'),
>   gettext('column 2')
> };
>
> ctrl.noItemMessage = gettext('You no longer have any items in your table.
> You either lack the sufficient priveledges or your search criteria is not
> valid');
>
> ctrl.batchActionList = [
>   { name: 'create', onclick: somefunction, etc.... }
>   { name: 'delete', onclick: somefunction, etc.... }
> ];
>
> ctrl.rowActionList = [
>   { name: 'edit', onclick: somefunction, etc.... }
>   { name: 'delete', onclick: somefunction, etc.... }
> ];
>
> table.html
> -----------
> <div ng-controller="table.controller.js as ctrl">
>   <horizon-table
>     headers="ctrl.headers"
>     batch-actions="ctrl.batchActionList"
>     row-actions="ctrl.rowActionList">
>   </horizon-table>
> </div>
>
> So now your controller is polluted with presentation and translation
> logic. In addition, we will have to live with long gettext messages and
> add eslint ignore rules just to pass it. The flip side is that you do
> have a simple directive that points to a common template sitting somewhere. It
> is not that much "easier" to the example below. What we're really doing is
> defining the same presentation logic, but in the HTML instead. Lastly, I'll
> bring up the customization again because many products are going to want to
> customize their tables. They maybe the minority but that doesn't mean we
> shouldn't support them.
>
> *Option 2:*
>
> table.html
> ------------
> <table ng-controller="table.controller.js as ctrl">
> <thead>
>   <tr>
>     <action-list>
>       <action callback="someFunc" translate>Create</action>
>       <action callback="someFunc" translate>Delete</action>
>     </action-list>
>   </tr>
>   <tr>
>     <th translate>Column 1</th>
>     <th translate>Column 2</th>
>   </tr>
> </thead>
> <tbody>
>   <tr ng-repeat="items in ctrl.items">
>     <td>....</td>
>     <td><action-list>
>       <action callback="someFunc" translate>Edit</action>
>       <action callback="someFunc" translate>Delete</action>
>     </action-list></td>
>   </tr>
> </tbody>
> </table>
>
> Here, your table.controller.js worries ONLY about data and data
> manipulation. The presentation logic all resides in the HTML. If I want to
> add icons in the table header, I can do that easily. Remember that this is
> plain HTML, this is a lot easier for someone new to come in and learn this
> than our special horizon-table directive. It is definitely easier to USE,
> but I would argue that it is harder to learn.
>
> --------------
>
> If you compare the two options above, you'll see that all we've really
> done is move presentation logic from the controller into the HTML. You have
> to define that logic somewhere, why not in the HTML? This makes it easier
> to read and know what you're going to see in the browser (something HTML5
> spec is evangelizing), and you get the bonus benefit of customization.
>
> I'd like to point out that we aren't getting rid of directives, we're
> still using directives them (like <action-list>, <action>, <magic-search>,
> etc..) in our tables. The pattern is, you build your panels using smaller
> components instead of having one giant component that encapsulates
> everything. Of course, there isn't a right or wrong answer, in fact there
> are two very different implementations of a table directive out there
> right now:
>
> http://ng-table.com (more inline with option 1)
> http://lorenzofox3.github.io/smart-table-website/ (more inline with
> option 2)
>
> Basically, what I'm trying to say is: let's build something simple and
> easy to understand first (small components that we work), then we can build
> something more complex on top of it so that it easier to use. I don't think
> there is a right or wrong answer, just two very different ways of thinking
> and implementation. But if we start with smaller components first, we get
> the goods of both world. The guys that want to customize will have a way to
> do it by bypassing the horizon-table directive, and the guys that just want
> a simple table can use the more complex directive.
>
> -----Lin Hua Cheng <os.lcheng at gmail.com> wrote: -----
> To: "OpenStack Development Mailing List (not for usage questions)" <
> openstack-dev at lists.openstack.org>
> From: Lin Hua Cheng <os.lcheng at gmail.com>
> Date: 08/19/2015 05:15PM
> Subject: Re: [openstack-dev] [Horizon] Update on Angular Identity work
>
> Hi Thai,
>
> Thanks for investigating the two options.
>
> Option 2 might be better. Folks have to learn the new pattern of writing
> multiple files, so I think the learning curve for a new table directive is
> not that much of a difference.
>
> I think option 2 is going to be easier to maintain, since we have a layer
> of abstraction. It might even also increase adoptability since it would be
> easier to use.  It might be harder to customize, but that would probably
> not be done often.  The table directive would be used as is most of the
> time.
>
> My thought is design the code to be easy to use for the use case that will
> be used most of the time rather than the customization case  which maybe
> harder to do. Which leads me to preferring option 2.
>
> Thanks,
> Lin
>
> On Wed, Aug 19, 2015 at 12:16 PM, Thai Q Tran <tqtran at us.ibm.com> wrote:
>
>> Hi Lin,
>>
>> I agree with you and Eric that we have a lot of HTML fragments. Some of
>> them I think make sense as directives:
>> The table footer is a good example of something we can convert into a
>> directive: https://review.openstack.org/#/c/207631/
>> The table header on the other hand is something more specific to your
>> table:
>> https://github.com/openstack/horizon/blob/master/openstack_dashboard/dashboards/identity/static/dashboard/identity/users/table/table-header.html
>>
>> So there are two approaches we can take here:
>> 1. Keep some of the presentation related data in the HTML: mainly things
>> like table headers, column definitions, translated texts, etc... I like
>> this approach a bit more because it allow us to read the HTML and know
>> exactly what we are expecting to see. This table.html is compose of smaller
>> directives like hz-table-footer and regular html tags like <th> and <td>
>> etc... I think as we have more of these smaller directives available, we
>> can combine the fragments into one file.
>>
>> 2. We could create a more general table directive with a common template.
>> This is more inline with what we have currently for legacy. BUT the
>> presentation logic like translations, definitions would now have to reside
>> in the table controller AND we lose the semantic readability part. Doing it
>> this way could potentially introduce more complexity as it now requires
>> people to learn the table directive, which could be very complex if it does
>> not use smaller directives. Another common problem we encountered with this
>> pattern was a lack of customization. In legacy, it was pretty hard to add
>> an icon into a table cell. If we go down this route, I believe we might
>> start to encounter the same issues.
>>
>> In summary, we are working on addressing the HTML fragments, but I think
>> we as a community should go with option 1 and stay away from option 2.
>>
>> -----Lin Hua Cheng <os.lcheng at gmail.com> wrote: -----
>> To: "OpenStack Development Mailing List (not for usage questions)" <
>> openstack-dev at lists.openstack.org>
>> From: Lin Hua Cheng <os.lcheng at gmail.com>
>> Date: 08/18/2015 02:36PM
>> Cc: Vince Brunssen/Austin/IBM at IBMUS
>> Subject: Re: [openstack-dev] [Horizon] Update on Angular Identity work
>>
>>
>> I think the table setup pattern have some opportunity for reducing code
>> duplication before it gets re-used by other panels..
>>
>> We used to just need to write one file to define a table, now we have to
>> write 9 files [1].  Can we have a table directive to reduce the duplicated
>> code before moving forward to other panels?
>>
>> -Lin
>>
>> [1]
>> https://github.com/openstack/horizon/tree/master/openstack_dashboard/dashboards/identity/static/dashboard/identity/users/table
>>
>> On Tue, Aug 18, 2015 at 11:49 AM, Thai Q Tran <tqtran at us.ibm.com> wrote:
>>
>>> Hi everyone,
>>>
>>> Just wanted to keep everyone up to date on the angular panels work. The
>>> goal was to set a pattern that others can follow, to that end, there were a
>>> few requirements:
>>> 1. reusable and possibly pluggable
>>> 2. easy to understand
>>> 3. reduce code duplication
>>>
>>> These requirements don't always go hand-in-hand, and that is the primary
>>> reason why it is taking a bit longer. I believe we are nearing the end of
>>> it, here are some items remaining that I believe is crucial to finishing up
>>> this work.
>>>
>>> a. i18n was completed, so we need help moving gettext blobs to HTML
>>> templates (example patch: https://review.openstack.org/#/c/210366/ )
>>> volunteers are welcomed! We want others to use the translate directive as
>>> the main way to translate text blobs, this was why we went down this road
>>> using babel and angular_extractor plugin.
>>>
>>> b. transfer table supports clone feature (
>>> https://review.openstack.org/#/c/211345/ ). There were a lot of
>>> template duplications, this clone feature reduces the HTML by a
>>> considerable amount. Since this is something we use quite often, it made
>>> sense to invest time into improving it. We have had complaints that there
>>> was too much HTML fragments, this will address a bit of that. One of the
>>> challenge was to get it working with existing launch-instance, so I spent
>>> about 2 weeks making sure it worked well with the old code while allowing
>>> the new clone feature.
>>>
>>> c. I believe we have a pretty good pattern setup for tables. The final
>>> piece of the puzzle was the patterns for various actions. We have wizard
>>> (create user), form (edit user), confirmation dialog (delete user), and
>>> actions with no modal dialog (enable user). We wanted a general pattern
>>> that would address the requirements mentioned above. There were some
>>> discussions around extensibility at the midcycle that I think will fit well
>>> here as well (
>>> https://blueprints.launchpad.net/horizon/+spec/angular-workflow-plugin
>>> ). The actions can follow a similar pattern to workflow. I believe this
>>> pattern would address #1 and #3 but making it easy to understand is a bit
>>> challenging - I think this is where documentation could help.
>>>
>>> https://review.openstack.org/#/c/202315/ and a few other patches are
>>> going to be ready for review soon (sometime end of this week)! Item #c
>>> is the most important piece, it is going to be the general pattern that
>>> people will use to build their angular panels with, so the more eyes we can
>>> get on it, the better. My aim is to get it in before the feature freeze
>>> and I think that is entirely possible with your help. So please help review
>>> even if you are not a core!
>>>
>>> Thanks
>>>
>>>
>>>
>>>
>>>
>>> __________________________________________________________________________
>>> OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe:
>>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>>
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject
>> :unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150820/b1491e19/attachment.html>