Details

      Description

      Currently many fields have 'widget-style="required" to indicate that input for the field is required. Better is it to have 'required-field="true". This not only opens up the possibility to define additional validation rules, but the rendered also incorporates the aspects associated with 'widget-style="required".

      1. OFBIZ-6690-removewidgetstyle.patch
        90 kB
        Taher Alkhateeb
      2. OFBIZ-6690-SetupForms.xml.patch
        12 kB
        Pierre Smits

        Issue Links

          Activity

          Hide
          pfm.smits Pierre Smits added a comment -

          This patch addresses the issue regarding SetupForms.xml

          Show
          pfm.smits Pierre Smits added a comment - This patch addresses the issue regarding SetupForms.xml
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Pierre, I counted in total about 140 instances of this modification. If we are going to apply it, perhaps we should do so across the board in all components in one shot. The attached patch can do that.

          Did you test this change though? Is there any effect in the translated HTML that we need to worry about?

          Show
          taher Taher Alkhateeb added a comment - Hi Pierre, I counted in total about 140 instances of this modification. If we are going to apply it, perhaps we should do so across the board in all components in one shot. The attached patch can do that. Did you test this change though? Is there any effect in the translated HTML that we need to worry about?
          Hide
          pfm.smits Pierre Smits added a comment -

          Hi Taher,

          I counted far less in the patch I provided, but you are right: there are more in other files.

          The related aspect in the renderer in htmlFormMacroLibrary is this:

          <#macro renderAsterisks requiredField requiredStyle>
            <#if requiredField=="true"><#if !requiredStyle?has_content>*</#if></#if>
          </#macro>
          

          And yes, I did test.

          Show
          pfm.smits Pierre Smits added a comment - Hi Taher, I counted far less in the patch I provided, but you are right: there are more in other files. The related aspect in the renderer in htmlFormMacroLibrary is this: <#macro renderAsterisks requiredField requiredStyle> <# if requiredField== " true " ><# if !requiredStyle?has_content>*</# if ></# if > </#macro> And yes, I did test.
          Hide
          taher Taher Alkhateeb added a comment -

          Okay, so on another search in the entire framework I see the following:

          item count in code
          required-field="true" 62
          widget-style="required" 140

          so this means most of the required fields are enforced with the widget-style format instead of the required-field format. I can also see something like the below code snippet which I think only happens in the required-field, not the styled one.

          So the question I have now is why do we have two ways of showing a field as required, and shouldn't we remove one of the two completely from the code base for consistency's sake? What do you think?

          <#macro renderFormClose focusFieldName formName containerId hasRequiredField>
            </form><#lt/>
            <#if focusFieldName?has_content>
              <script language="JavaScript" type="text/javascript">
                var form = document.${formName};
                form.${focusFieldName}.focus();
                <#-- enable the validation plugin for all generated forms
                only enable the validation if min one field is marked as 'required' -->
                if (jQuery(form).find(".required").size() > 0) {
                  jQuery(form).validate();
                }
              </script><#lt/>
            </#if>
            <#if containerId?has_content && hasRequiredField?has_content>
              <script type="text/javascript">
                jQuery("#${containerId}").validate({
                  submitHandler:
                    function(form) {
                      form.submit();
                    }
                });
              </script>
            </#if>
          </#macro>
          
          Show
          taher Taher Alkhateeb added a comment - Okay, so on another search in the entire framework I see the following: item count in code required-field="true" 62 widget-style="required" 140 so this means most of the required fields are enforced with the widget-style format instead of the required-field format. I can also see something like the below code snippet which I think only happens in the required-field, not the styled one. So the question I have now is why do we have two ways of showing a field as required, and shouldn't we remove one of the two completely from the code base for consistency's sake? What do you think? <#macro renderFormClose focusFieldName formName containerId hasRequiredField> </form><#lt/> <# if focusFieldName?has_content> <script language= "JavaScript" type= "text/javascript" > var form = document.${formName}; form.${focusFieldName}.focus(); <#-- enable the validation plugin for all generated forms only enable the validation if min one field is marked as 'required' --> if (jQuery(form).find( ".required" ).size() > 0) { jQuery(form).validate(); } </script><#lt/> </# if > <# if containerId?has_content && hasRequiredField?has_content> <script type= "text/javascript" > jQuery( "#${containerId}" ).validate({ submitHandler: function(form) { form.submit(); } }); </script> </# if > </#macro>
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I agree about your conclusions. We should replace all instances and completely remove <<widget-style="required">> from code generation. I guess we have both for history reasons. I checked themes have no major impacts on this.

          About "HTML" generation, for instance at catalog/control/EditProductStore?productStoreId=9000 the only difference is 1 char: the "*"

          Show
          jacques.le.roux Jacques Le Roux added a comment - I agree about your conclusions. We should replace all instances and completely remove <<widget-style="required">> from code generation. I guess we have both for history reasons. I checked themes have no major impacts on this. About "HTML" generation, for instance at catalog/control/EditProductStore?productStoreId=9000 the only difference is 1 char: the "*"
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Pierre, Taher

          Taher your slightly modified is committed at revision: 1709872

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Pierre, Taher Taher your slightly modified is committed at revision: 1709872
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I reopen because we need to deprecate and remove the required-field-style attribute

          Show
          jacques.le.roux Jacques Le Roux added a comment - I reopen because we need to deprecate and remove the required-field-style attribute
          Hide
          taher Taher Alkhateeb added a comment -

          I am not sure that we need to change anything else Jacques. We just need to stop manually assigning a CSS class using widget-style. In other words, we are just replacing manual CSS assignment with automatic one that comes from the required-field="true" attribute being set and translated at the template level.

          Or maybe I'm missing something? Not sure.

          Show
          taher Taher Alkhateeb added a comment - I am not sure that we need to change anything else Jacques. We just need to stop manually assigning a CSS class using widget-style. In other words, we are just replacing manual CSS assignment with automatic one that comes from the required-field="true" attribute being set and translated at the template level. Or maybe I'm missing something? Not sure.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Yes right, we can keep required-field-style which is not used OOTB but could find an use in custom project. I re-close

          Show
          jacques.le.roux Jacques Le Roux added a comment - Yes right, we can keep required-field-style which is not used OOTB but could find an use in custom project. I re-close
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Damn, I want this "Done" not "Fixed"

          Show
          jacques.le.roux Jacques Le Roux added a comment - Damn, I want this "Done" not "Fixed"

            People

            • Assignee:
              taher Taher Alkhateeb
              Reporter:
              pfm.smits Pierre Smits
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development