? ? Pending

User tests: Successful: Unsuccessful:

avatar YatharthVyas
YatharthVyas
3 May 2021

Pull Request for #33176 (comment)
Thanks for bringing it into my attention @regularlabs

Summary of Changes

Reverted $divClass back to $class for styling the outer div

Testing Instructions

Make a new plugin XML form with a radio field or use an existing one for example: /plugins/content/vote/vote.xml

Replace the Radio field with the following code:

<field
	name="show_total_votes"
	type="radio"
	label="PLG_VOTE_TOTAL_VOTES_LABEL"
	class=""
	default="0"
	filter="integer"
>
	<option value="0">JHIDE</option>
	<option value="1">JSHOW</option>
</field>

Try out the following combinations for the class="" attribute: (Feel free to replace text-muted with any other custom class)

  1. "btn-group"
  2. "btn-group text-muted"
  3. "btn-group btn-group-yesno"
  4. "btn-group btn-group-yesno text-muted"
  5. "text-muted"
  6. And lastly, remove the class attribute and check

Actual result BEFORE applying this Pull Request

The Custom Class (text-muted) will not affect the output and it solely depends on btn-group and btn-group-yesno

Expected result AFTER applying this Pull Request

The custom class will also be taken into consideration

custom.radio.mp4

Documentation Changes Required

  1. In Joomla 4 Docs Plugin Tutorial Page, the XML form has mentioned class="switcher" for the radio field. This class is redundant and doesn't affect anything. I believe the author intended to write layout="joomla.form.field.radio.switcher"

  2. While studying the code, I found out that classes, (Edit: also onchange and onclick) can also be applied to the <option> as <option class="text-muted"> under the radio field. For example:

<field
	name="show_total_votes"
	type="radio"
	label="PLG_VOTE_TOTAL_VOTES_LABEL"
	default="0"
	filter="integer"
>
	<option value="0" class="text-muted">JHIDE</option>
	<option value="1">JSHOW</option>
</field>

will dislpay the output as:

image

The documentations doesn't mention this: (only attribute given here is value) so a new attribute for class (Edit: also onchange and onclick) must be mentioned here

image

Link: https://docs.joomla.org/Radio_form_field_type

The code that is responsible for checking the option tag's classes is:

$optionClass = !empty($option->class) ? $option->class : $btnClass;

avatar YatharthVyas YatharthVyas - open - 3 May 2021
avatar YatharthVyas YatharthVyas - change - 3 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 3 May 2021
Category Layout
avatar YatharthVyas
YatharthVyas - comment - 3 May 2021

appveyor build is failing because the link https://getcomposer.org/composer-1.phar seems to be broken at the moment. I don't think any of the changes in my PR are the culprit for this one ?

avatar regularlabs
regularlabs - comment - 3 May 2021

Fix looks good ?

However, I do feel there is way too much magic going on in this file.
Personally, I think the HTML output should not rely on the class names.
A class name should just be that. Let CSS handle it from there.

Wouldn't it make more sense to just make an extra layout file for the 'btn-group' version, like there is one for the switcher?

avatar YatharthVyas
YatharthVyas - comment - 3 May 2021

Making a new layout seems to be a tempting fix however, that would imply the usage of something on the lines of layout="joomla.form.field.radio.btngroup" attribute for this and it might cause issues in terms of backwards compatibility for all the fields out there that are working on class="btn-group"

avatar regularlabs
regularlabs - comment - 3 May 2021

Then maybe add something like this to the top of the default layout:

if(strpos($class, 'btn-group')) {
   ... code to load the 'group' layout ...
   return;
}
avatar regularlabs
regularlabs - comment - 3 May 2021

And maybe even have a separate yesno layout too:

if(strpos($class, 'btn-group-yesno')) {
   ... code to load the 'yesno' layout ...
   return;
}
if(strpos($class, 'btn-group')) {
   ... code to load the 'group' layout ...
   return;
}
avatar YatharthVyas YatharthVyas - change - 3 May 2021
The description was changed
avatar YatharthVyas YatharthVyas - edited - 3 May 2021
avatar YatharthVyas
YatharthVyas - comment - 3 May 2021

If I were to separate these out then the code would, of course, be more readable. However, because the underlying HTML markup and attributes except for the class are identical for all the cases, a lot of the code would be repeated in the same file, which in my opinion, isn't a good practice. This tantamounts to being a comparison between better code readability v/s less redundancy. Both have their pros but I'm biased to lean towards less redundancy here as it implies fewer changes in the code and is backed by the assurance that things are working as intended in the current state.

avatar regularlabs
regularlabs - comment - 3 May 2021

Ok.

avatar YatharthVyas YatharthVyas - change - 5 May 2021
Labels Added: ?
avatar ceford ceford - test_item - 8 May 2021 - Tested successfully
avatar ceford
ceford - comment - 8 May 2021

I have tested this item successfully on ac524e0

Styling works as described. I fixed the Creating a Plugin for Joomla tutorial to remove class switcher and use layout instead. I left the Radio form field type tutorial alone because ...


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

avatar Quy Quy - test_item - 11 May 2021 - Tested successfully
avatar Quy
Quy - comment - 11 May 2021

I have tested this item successfully on ac524e0


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

avatar Quy Quy - change - 11 May 2021
Status Pending Ready to Commit
avatar Quy
Quy - comment - 11 May 2021

RTC


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

avatar richard67 richard67 - close - 12 May 2021
avatar richard67 richard67 - merge - 12 May 2021
avatar richard67 richard67 - change - 12 May 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-05-12 10:08:01
Closed_By richard67
Labels Added: ?
avatar richard67
richard67 - comment - 12 May 2021

Thanks!

avatar YatharthVyas
YatharthVyas - comment - 12 May 2021

Thanks everyone!
Especially @ceford for the documentation fix

avatar machadoug
machadoug - comment - 8 Jun 2021

Thank you @YatharthVyas for this PR.
I just noticed the active class doesn't change when I click on another option.

I've added your code to /plugins/content/vote/vote.xml for testing purposes and it didn't work. See:

active-btn-group.mp4

Code added:

<field
	name="show_total_votes"
	type="radio"
	label="PLG_VOTE_TOTAL_VOTES_LABEL"
	default="0"
	filter="integer"
	class="btn-group btn-group-yesno"
>
	<option value="0" >JHIDE</option>
	<option value="1">JSHOW</option>
</field>

I'm not sure where/how to fix it and whether I should create a new issue or not.

avatar YatharthVyas
YatharthVyas - comment - 9 Jun 2021

@machadoug Thank you for informing me about this. I'm afraid I might be doing something wrong because I was unable to replicate this in my local environment:

custom-class

Note:

The code provided by you seems perfect and you've done everything correctly in the demo so there is no mistake made by you. However, I observed that the UI in the video you shared seems to belong to an older version of Joomla (before the Admin UI was repainted) so I think the issue might be occurring here because it is running an older version of J4.

Could you please try to update your build to the latest commit/nightly? I have a feeling it might resolve the issue.

Please notify me in case the issue persists after updating.

System Information:

image

My J4 Version is updated to the latest commit: 317f826

avatar machadoug
machadoug - comment - 9 Jun 2021

Thank you @YatharthVyas.
The problem was fixed using the latest nightly build.

Add a Comment

Login with GitHub to post a comment