User tests: Successful: Unsuccessful:
Pull Request for #33176 (comment)
Thanks for bringing it into my attention @regularlabs
Reverted $divClass
back to $class
for styling the outer div
Make a new plugin XML form with a radio
field or use an existing one for example: /plugins/content/vote/vote.xml
joomla-cms/plugins/content/vote/vote.xml
Line 33 in 8a1d803
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)
The Custom Class (text-muted) will not affect the output and it solely depends on btn-group
and btn-group-yesno
The custom class will also be taken into consideration
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"
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:
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
Link: https://docs.joomla.org/Radio_form_field_type
The code that is responsible for checking the option tag's classes is:
Status | New | ⇒ | Pending |
Category | ⇒ | Layout |
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?
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"
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;
}
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;
}
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.
Ok.
Labels |
Added:
?
|
I have tested this item
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 ...
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
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:
?
|
Thanks!
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:
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.
@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:
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.
My J4 Version is updated to the latest commit: 317f826
Thank you @YatharthVyas.
The problem was fixed using the latest nightly build.
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?