Thank you Jacques,

here is some quick feedback after a review of the patch.

A) all the code in framework/images/webapp/images/fieldlookup.js is bad because 
contains hardcoded application/components

B) what is this?

Index: specialpurpose/birt/ofbiz-component.xml
===================================================================
--- specialpurpose/birt/ofbiz-component.xml     (revision 1407381)
+++ specialpurpose/birt/ofbiz-component.xml     (working copy)
@@ -29,7 +29,6 @@
     <entity-resource type="data" reader-name="seed" loader="main" 
location="data/OrderPortletData.xml"/>
     <service-resource type="model" loader="main" 
location="servicedef/services.xml"/>
    
-   <!-- use when reports need to be injected into applications Note: this will 
break context help for those applications.
     <webapp name="accounting"
             title="Accounting"
             server="default-server"
@@ -50,7 +49,6 @@
             location="webapp/ordermgr"
             base-permission="OFBTOOLS,ORDERMGR"
             mount-point="/ordermgr"/>
-    -->
     <webapp name="birt"
             title="BIRT"
             server="default-server"

C) I still think it is a bad idea to add dependencies to the content component 
on other applications like: applications/content/webapp/ofbizhelp/catalog_en

D) did you check the compliance with licenses? See this for example:

+/*----------------------------------------------------------------------------
+ * JavaScript for webhelp search
+ *----------------------------------------------------------------------------
+ This file is part of the webhelpsearch plugin for DocBook WebHelp
+ Copyright (c) 2007-2008 NexWave Solutions All Rights Reserved.
+ www.nexwave.biz Nadege Quaine
+ http://kasunbg.blogspot.com/ Kasun Gajasinghe
+ */

E) how was generated the content of (for example):

Index: 
applications/content/webapp/ofbizhelp/catalog_en/content/search/htmlFileInfoList.js

? How should we maintain it?

F) why this:

applications/content/webapp/ofbizhelp/catalog_en/common/jquery/jquery-1.4.2.min.js

?

I think it is enough for now, but the changes are big and I couldn't review 
everything.

In general, my preference would be to see this type of contribution being 
implemented as a pluggable feature (with data mainatined externally in 
Confluence or in a specialpurpose or extra component) rather than being part of 
the trunk.

Kind regards,

Jacopo


On Nov 10, 2012, at 12:44 PM, Jacques Le Roux wrote:

> The 4/5 last comments are enough to read, before were mostly exchange between 
> Tom and I. Adrian's concern is addressed in those last comments.
> I have just attache a OFBIZ-4941.patch corresponding to what will be actually 
> committed. This will replace the current help system and will be completed by 
> Tom. He clearly said that he is committed to finish the work, and I trust 
> him: most is done already.
> 
> In doubt, sorting attachments by date helps.
> 
> Thanks for your comments and opinion :)
> 
> Jacques
> 
> From: "Jacopo Cappellato" <[email protected]>
>> The log history is huge and messed up and there are several files attached 
>> to the Jira task: this makes it difficult to review the work that you would 
>> like to commit.
>> Having a summary of the code that is going to be contributed and a final 
>> list of files/diffs would help me to take a decision... but hopefully others 
>> have been more involved in the details to express their opinion; are you 
>> also going to remove the existing help system or this new one will be added 
>> as an alternative option? In my opinion we can't have the luxury to maintain 
>> two help systems in OFBiz.
>> 
>> Kind regards,
>> 
>> Jacopo
>> 
>> 
>> On Nov 10, 2012, at 12:17 PM, Jacques Le Roux wrote:
>> 
>>> If nobody disagree I will commit 
>>> https://issues.apache.org/jira/browse/OFBIZ-4941 soon
>>> 
>>> Jacques
>> 
>> 

Reply via email to