[#8473] - Follow up on #8085 - refactor com_contact form to support renderField
- Closed
- 6 Dec 2015
- Medium
- Build: staging
- Easy Test
- # 8473
- Diff
- matrikular:patch-2
- Error continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error Details
User tests: Successful: Unsuccessful:
Introduction
As a follow-up on #8085 I would like to propose a few changes first to com_contact and later on to com_users as well.
While there are a couple of code style changes in files that are not directly related to com_contact, I thought it would be good to include them in this PR since they were edited in the same context.
Status quo
The last PR allows us to override and / or add fields to the contact form via plugins without the need to create a template override because the rendering is based on the xml file. As discussed in the comment section in #8085, we normally want to use the renderField method instead of writing things like echo $field->input. For that, a component specific renderfield override had to be created, which I did with this PR.
The method used to check for a required state and / or field of the type label is not very clean but should suffice for now. I normally would have created or rewritten the fields and their layouts but that would have been too much for this PR and, as @wilsonge told me, that is planned for the near future. Take a look at layouts/joomla/form/field/radio.php to see where this is going.
Changes
I've tried to document / describe the changes made to every file within the description of the file change request. Without going too much into the details, let me please give you a short summary:
- Refactor the form layout to make use of the renderField method. In doing so, the legend for each fieldset (if present) will be taken from the xml file and the markup for each field will be based on the renderfield.php and the respective layout. As a bonus, the contact form now supports the showon-Attribute.
- Some language strings, like the fieldset label and the "Send copy to yourself" have been changed.
- The form markup was changed to match com_users.registration and login which includes adding a "well" CSS class suffix to the form.
Before
After
My goal was to unify the output / markup, to fix some bugs and add some functionality to all of the forms of com_contact and com_users (registration / login).
Long story short, how to test?
Well, first of all, this only works for the latest dev branch. Second, after applying the patch the output should look like in the second picture. Apart from that you can follow the test instructions mentiond in #8085. I've uploaded a new version of the test plugin here.
If you have any questions, suggestions or corrections feel free to contact me.
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
?
|
||
Tested on FF 40 - the output looks like the second screenshot. An empty <fieldset></fieldset> is in the html code, if no Captcha is used.
@matrikular looks good here i can confirm the findings by @ufuk-avcu as well as i agree to use the nativ and not the string helper strlen function.
I can confirm all works bevor as expected. Great work 
Thank you for your comments and tests.
I replaced the StringHelper::strlen method with the nativ strlen function and extended the foreach loop to only render the fieldset when captcha was enabled.
It would mean a lot to me if you could test it again, please.
This PR has received new commits.
CC: @bembelimen
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8473.
I have tested this item 
successfully on a16cc17
successful on 3.5
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8473.
By adding the well class to the form you are potentially changing the display on existing web sites not sure if that passes the B/C promise
@brianteeman thank you for your input.
I would like to see the addition of the well class suffix to the form more of a bug fix than a potential b/c issue. If we look at the registration and login view of com_users e.g, both forms have the well suffix. Adding it would add to the general user experience (forms look like "this" ~> POLS).
I've checked approx. 50 sites (google / j.org showcase) where this would have no negative impact.
It would change the layout on all sites that I have built. Not sure how happy I am about that.
Templates aren't covered by our B/C policy. So this PR doesn't have to be backward compatible in this regard.
Adding the well sure will not break any template functionality. It will obviously change appearance on most templates. The real question in the end thus is if it is considered an improvement or not and if it's worth it.
My2Cent: I always make overrides for the contact form... because it is ugly as it is, with the well only around the button. It is an improvement :)
rdeutz
- | Milestone |
Added: |
||
| Category | ⇒ | Components |
| Labels | |||
| Easy | No | ⇒ | Yes |
Tested it as described. Works as expected. Fine work.
This PR has received new commits.
CC: @bembelimen, @chmst
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8473.
As far as I can tell, it's not this PR that fails, correct? (~PHP7 build)
Yeah, it's an issue related to a hardware outage on pear.net. See travis-ci/travis-ci#5207
Tests pass in the non-PHP 7 libraries and there are two good tests here. merging
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-12-06 00:20:25 |
| Closed_By | ⇒ | wilsonge |
Also, a string has been deleted
COM_CONTACT_FORM_LABEL="Send an Email. All fields with an asterisk (*) are required.
This is not B/C as the language packs can be used on pre-3.5.0 installs.
I am making a PR for that.
| Milestone |
Added: |
||
| Milestone |
Removed: |
||
I have tested this item
successfully on e119c7f
Two general comments:
Otherwise the PR looks very good. Awesome work!
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8473.