OFBiz
  1. OFBiz
  2. OFBIZ-4926

renderLookupField macro: get a form element by "ID" instead of "Name"

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: SVN trunk
    • Fix Version/s: SVN trunk
    • Component/s: framework
    • Labels:
      None
    • Environment:

      Ubuntu 10.04

      Description

      Hello all,

      We have a problem to render a lookup field in a form element without a "name" attribute.
      We need to improve a lookup field because if we render the field without form or different form then the lookup field doesn't work, so we will change the lookup field by not get a from element by a "name" attribute but get by an "id" attribute.

      Regards,
      Chatree Srichart

      1. lookup.patch
        5 kB
        Chatree Srichart

        Activity

        Hide
        Jacques Le Roux added a comment -

        Reading your description (did not look at code) it does not make sense to me: there is no id attribute to a form element http://www.w3.org/TR/html401/interact/forms.html#h-17.3
        And why would we change that OOTB? There are already a lot of lookups working that way.

        Show
        Jacques Le Roux added a comment - Reading your description (did not look at code) it does not make sense to me: there is no id attribute to a form element http://www.w3.org/TR/html401/interact/forms.html#h-17.3 And why would we change that OOTB? There are already a lot of lookups working that way.
        Hide
        Chatree Srichart added a comment -

        Hello Jacques,

        I am sorry to make you confuse. I think I make confusing with my description. Let me correct myself.

        What I would like to change the renderLookupField macro is changing the reference to the lookup element:

        from: document.$

        {formName}

        .$

        {fieldName}

        to: document.getElementById("$

        {id}

        ") which "id" is an id attribute of the lookup element.

        because sometimes I want to render the lookup field without form element.
        To change this will not cause a problem with the lookup fields which already used in the system because there always be "id" for the lookup fields.

        Regards,
        Chatree Srichart

        Show
        Chatree Srichart added a comment - Hello Jacques, I am sorry to make you confuse. I think I make confusing with my description. Let me correct myself. What I would like to change the renderLookupField macro is changing the reference to the lookup element: from: document.$ {formName} .$ {fieldName} to: document.getElementById("$ {id} ") which "id" is an id attribute of the lookup element. because sometimes I want to render the lookup field without form element. To change this will not cause a problem with the lookup fields which already used in the system because there always be "id" for the lookup fields. Regards, Chatree Srichart
        Hide
        Jacques Le Roux added a comment -

        Hi Chatree,

        After reading the patch and your explanation, I have still a concern. The lookup mechanism needs a form name in order to render the description besides the lookup. You removed the check which else shows <<alert("Developer: for lookups to work you must provide a form name!")>>. I think we should still keep it. You could remove it in your custom application if you want. You will just lose the description but this is not a blocked.

        Apart that I agree that it should works OOTB and my few tests where positive. So I think I will commit your patch but will keep the form name check OOTB.

        Show
        Jacques Le Roux added a comment - Hi Chatree, After reading the patch and your explanation, I have still a concern. The lookup mechanism needs a form name in order to render the description besides the lookup. You removed the check which else shows <<alert("Developer: for lookups to work you must provide a form name!")>>. I think we should still keep it. You could remove it in your custom application if you want. You will just lose the description but this is not a blocked. Apart that I agree that it should works OOTB and my few tests where positive. So I think I will commit your patch but will keep the form name check OOTB.
        Hide
        Jacopo Cappellato added a comment -

        Jacques,

        please do extensive testing if you are planning to commit this; if you do not have time then ask the review/tests from others and wait until the test reports are successful.

        Thanks

        Show
        Jacopo Cappellato added a comment - Jacques, please do extensive testing if you are planning to commit this; if you do not have time then ask the review/tests from others and wait until the test reports are successful. Thanks
        Hide
        Chatree Srichart added a comment -

        Better do not commit the patch yet because it will not help for me if the alert() still show. Let me find another solution. However, if you think it will be useful for the framework that is fine.

        Regards,
        Chatree Srichart

        Show
        Chatree Srichart added a comment - Better do not commit the patch yet because it will not help for me if the alert() still show. Let me find another solution. However, if you think it will be useful for the framework that is fine. Regards, Chatree Srichart
        Hide
        Jacques Le Roux added a comment -

        Jacopo,

        Actually, I'm still reluctant to commit. That's why I did not this morning. After an hour of review and test, I'm convinced that it will continue to work well with this patch, if we keep the form name check OOTB. Chatree could then remove it from his appplications. Reviews and tests are always welcome.

        Chatree,

        As Japoco outlined, it introduces a risk and for now I prefer to put this aside because it adds not much benefits. In which cases do you need to have a loopud field outside of a form? Can't you then use a dummy form?

        Show
        Jacques Le Roux added a comment - Jacopo, Actually, I'm still reluctant to commit. That's why I did not this morning. After an hour of review and test, I'm convinced that it will continue to work well with this patch, if we keep the form name check OOTB. Chatree could then remove it from his appplications. Reviews and tests are always welcome. Chatree, As Japoco outlined, it introduces a risk and for now I prefer to put this aside because it adds not much benefits. In which cases do you need to have a loopud field outside of a form? Can't you then use a dummy form?

          People

          • Assignee:
            Unassigned
            Reporter:
            Chatree Srichart
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Time Tracking

              Estimated:
              Original Estimate - 168h
              168h
              Remaining:
              Remaining Estimate - 168h
              168h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development