? Pending

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
10 Apr 2017

Pull Request for Issue #15183 .

Summary of Changes

Taking care of the case where we have no formcontrol but a group.

Testing Instructions

  • Try showon in frontend template editing (eg Protostar: "Google Font for Headings")
  • Try showon in various other places

Expected result

Showon works as expected in all places

Actual result

Showon doesn't work in frontend template editing

Documentation Changes Required

None

avatar Bakual Bakual - open - 10 Apr 2017
avatar Bakual Bakual - change - 10 Apr 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 Apr 2017
Category Libraries
avatar brianteeman brianteeman - test_item - 10 Apr 2017 - Tested successfully
avatar brianteeman
brianteeman - comment - 10 Apr 2017

I have tested this item successfully on a12fd37


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

avatar AlexRed
AlexRed - comment - 10 Apr 2017

ok for Protostar: "Google Font for Headings"
but in frontend "Site Settings" --> SEO Settings --> Search Engine Friendly URLs
if yes don't show the other Seo settings as in Global configuration (Use URL Rewriting, Adds Suffix to URL, Unicode Aliases)


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

avatar brianteeman
brianteeman - comment - 10 Apr 2017

@AlexRed the fields you are talking about do not exist in that form so that form is working as intended. there is no showon stuff in that form https://github.com/joomla/joomla-cms/blob/staging/components/com_config/model/form/config.xml So if you feel that those fields should exist in that view it is a different issue to showon that this PR fixes

avatar AlexRed
AlexRed - comment - 10 Apr 2017

ok, sorry

avatar AlexRed AlexRed - test_item - 10 Apr 2017 - Tested successfully
avatar AlexRed
AlexRed - comment - 10 Apr 2017

I have tested this item successfully on a12fd37

Patch ok for me.


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

avatar Bakual
Bakual - comment - 10 Apr 2017

@brianteeman is right here in that this is the main reason it doesn't work 😄
However even if you do add the missing fields, it would still not work,
You would have to change those lines

<?php foreach ($this->form->getFieldset('seo') as $field) : ?>
	<div class="control-group">
		<div class="control-label"><?php echo $field->label; ?></div>
		<div class="controls"><?php echo $field->input; ?></div>
	</div>
<?php endforeach; ?>

(https://github.com/joomla/joomla-cms/blob/staging/components/com_config/view/config/tmpl/default_seo.php#L14-L23)
to the single line
<?php echo $this->form->renderFieldset('seo'); ?>

avatar franz-wohlkoenig franz-wohlkoenig - change - 10 Apr 2017
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 10 Apr 2017

RTC after two successful tests.

avatar wilsonge
wilsonge - comment - 10 Apr 2017

Just on a quick code review and no more (so might be talking rubbish) - is it possible to have a form control and group with the same name, in that case would this start to cause issues?

avatar Bakual
Bakual - comment - 10 Apr 2017

Just on a quick code review and no more (so might be talking rubbish) - is it possible to have a form control and group with the same name, in that case would this start to cause issues?

The formcontrol usually is just "jform". If we have both the group and form with the same name, it should still work but if we have one form without groups and another one without formcontrol and they have the same names, then it may become an issue.
In theory, it may be possible. But I doubt they would appear on the same page in practice. Also there will be likely other issues as well.
Usually we only have one form on a page so this should not become an issue.

avatar rdeutz rdeutz - change - 10 Apr 2017
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-04-10 15:09:44
Closed_By rdeutz
Labels Added: ?
avatar rdeutz rdeutz - close - 10 Apr 2017
avatar rdeutz rdeutz - merge - 10 Apr 2017

Add a Comment

Login with GitHub to post a comment