Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Follow up on #8085 - refactor com_contact form to support renderField #8473

Merged
merged 13 commits into from Dec 6, 2015

Conversation

Projects
None yet
@matrikular
Copy link
Contributor

matrikular commented Nov 17, 2015

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
contact_before
After
contact_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.

matrikular added some commits Nov 17, 2015

Refactor the form layout to make use of the renderField method
The following changes were made to this file, starting from the top:
- Removed the error-handle code block at the top that seemed to be left over from Joomla! 1.6
- Added the well CSS class suffix to the form to match the output / markup of com_users.registration and login
- Rewritten the foreach loop to fetch the fieldset and field information from the xml file and make use of the renderField method
- Adjusted the markup containing the button and hidden field/s to match the output / markup of com_users.registration and login
Remove unnecessary jimport and unused "com_media" params
The following changes were made to this file, starting from the top:
- Removed the com_media variable assignment since there seem to be no reference where this was used
- Removed unnecessary jimport for jhtml bootstrap
Add component specific renderfield JLayout override
This adds a component specific renderfield JLayout override. The "optional" addition to the label for required fields was moved from the com_contact form layout in here to be able to use the renderField method in the layout file.
Change contact form fieldset label to match the com_users schema
Change contact form fieldset label to match the com_users schema and add component specific "* Requied field" "constant"
Add spacer and update fieldset label format to match what is used in …
…com_users

Besides reordering the label and description attributes in this file which, to be honest, was more about finding peace and satisfying my OCD, I've changed the fieldset label to match the format that is used in com_users and added a spacer label to output the *Required field hint for the form.
Introduce extract($displayData); to make the variables more accessible
These changes allow a more direct access to input, label and the options
Introduce extract($displayData); to make the variables more accessible
As well es for the renderfield.php the proposed changes allow a more direct access the the variables used in this file
<?php if ($field->name === 'contact_email_copy' && !$this->params->get('show_email_copy')) : ?>
<?php continue; ?>
<form id="contact-form" action="<?php echo JRoute::_('index.php'); ?>" method="post" class="form-validate form-horizontal well">
<?php foreach ($this->form->getFieldsets() as $fieldset): ?>

This comment has been minimized.

Copy link
@zero-24

zero-24 Nov 17, 2015

Contributor

Can you add a space between $fieldset) and : ?>

@bembelimen

This comment has been minimized.

Copy link
Contributor

bembelimen commented Nov 18, 2015

I have tested this item successfully on e119c7f

Two general comments:

  • I saw, the new way to gather variables is "extract" (used in several other layouts/tmpl), in personal I think, that's not a solid way to code (often mentioned in "bad coding practises). Perhaps that function should be reviewed in Joomla! general.
  • If we want to check if a string is not empty, we don't need the StringHelper::strlen() method, strlen is enough (also on utf8 strings), perhaps a (very) small performance improvement

Otherwise the PR looks very good. Awesome work!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8473.
@ufuk-avcu

This comment has been minimized.

Copy link

ufuk-avcu commented Nov 18, 2015

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.

@zero-24

This comment has been minimized.

Copy link
Contributor

zero-24 commented Nov 18, 2015

@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 👍

@matrikular

This comment has been minimized.

Copy link
Contributor Author

matrikular commented Nov 19, 2015

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.

@joomla-cms-bot

This comment has been minimized.

Copy link

joomla-cms-bot commented Nov 20, 2015

This PR has received new commits.

CC: @bembelimen


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8473.

@chmst

This comment has been minimized.

Copy link
Contributor

chmst commented Nov 20, 2015

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.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Nov 21, 2015

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

@matrikular

This comment has been minimized.

Copy link
Contributor Author

matrikular commented Nov 21, 2015

@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.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Nov 21, 2015

It would change the layout on all sites that I have built. Not sure how happy I am about that.

@Bakual

This comment has been minimized.

Copy link
Contributor

Bakual commented Nov 21, 2015

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.

@chmst

This comment has been minimized.

Copy link
Contributor

chmst commented Nov 21, 2015

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 rdeutz added this to the Joomla 3.5.1 milestone Nov 22, 2015

@kolvar

This comment has been minimized.

Copy link

kolvar commented Nov 27, 2015

Tested it as described. Works as expected. Fine work.

Merge branch 'staging' of https://github.com/joomla/joomla-cms into p…
…atch-2

Conflicts:
	components/com_contact/views/contact/tmpl/default.php
@joomla-cms-bot

This comment has been minimized.

Copy link

joomla-cms-bot commented Dec 4, 2015

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.

@matrikular

This comment has been minimized.

Copy link
Contributor Author

matrikular commented Dec 5, 2015

As far as I can tell, it's not this PR that fails, correct? (~PHP7 build)

@Bakual

This comment has been minimized.

Copy link
Contributor

Bakual commented Dec 5, 2015

Yeah, it's an issue related to a hardware outage on pear.net. See travis-ci/travis-ci#5207

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Dec 6, 2015

Tests pass in the non-PHP 7 libraries and there are two good tests here. merging

wilsonge added a commit that referenced this pull request Dec 6, 2015

Merge pull request #8473 from matrikular/patch-2
Follow up on #8085 - refactor com_contact form to support renderField

@wilsonge wilsonge merged commit 520f02d into joomla:staging Dec 6, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Dec 6, 2015

@wilsonge
Please change Milestone as this is now in staging...

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Dec 6, 2015

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.

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Dec 6, 2015

Please merge on review #8602 (also correcting alpha order)

@Bakual Bakual modified the milestones: Joomla! 3.5.0, Joomla 3.5.1 Dec 6, 2015

matrikular added a commit to matrikular/joomla-cms that referenced this pull request Mar 25, 2016

Add a better check to the renderfield layout of com_contact to determ…
…ine whether or not a field is required

As mentioned in joomla#8473 (joomla#8473), ... as long as we cannot access the field properties properly, this seems to  be the way to go for now.

@matrikular matrikular deleted the matrikular:patch-2 branch Jul 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.