Public Forums 71,145 posts

[cheesegrits Friend cheesegritsJoin date: August 2007Posts: 10Downloads:5Uploads:0 Thanked: 1 times in 1 posts]

#1082238

Hey guys,

I’m the developer of Fabrik for Joomla, which has a lot of UI code.

I just noticed that in the latest release of the T3 framework, you have changed how the "on off" button groups are built (t3onoff, the case where there is a simple two button group with 1 and 0 as the values).

In line 27 of frontend-edit.js, you do this when converting radio buttons in fieldsets:

            var $this = $(this), $input = $this.prev('input'),
            cls = $this.hasClass('off') || $input.val() == '0' ? 'off' : 'on';

… which makes the assumption that the ‘input’ tag for the radio button is before the label, rather than in or after it. Which is often not the case.

This has broken all two button radio groups that Fabrik builds.

I’m working on some layout overrides in Fabrik to fix this, but can I suggest that rather than making assumptions about the DOM structure, you use the ‘for’ attribute on the label to get the input, which is what the ‘for’ attribute is for, so you don’t have to make assumptions about the DOM structure. And maybe a fallback to current behavior if the label doesn’t have a ‘for’ attribute …

var $this = $(this), $input = $('#' + $(this).attr('for'));
if ($input.length === 0) {
   $input = $this.prev('input');
};

Thanks for considering it,

— hugh

Hugh Messenger
Lead Dev
http://fabrikar.com

[cheesegrits Friend cheesegritsJoin date: August 2007Posts: 10Downloads:5Uploads:0 Thanked: 1 times in 1 posts]

#1082249

I had a look at changing Fabrik’s layouts in an override for T3, but for a lot of reasons it’s just not practical.

Also, I noticed that on line 36 of the same file, you already use the ‘for’ attribute rather than relying on DOM structure, for the exact same purpose …

            var label = $(this),
                input = $('#' + label.attr('for'));

… so I’m really hoping you’ll modify the code on line 27 to do the same thing. I changed my copy on a test server, and everything works fine.

— hugh

[cheesegrits Friend cheesegritsJoin date: August 2007Posts: 10Downloads:5Uploads:0 Thanked: 1 times in 1 posts]

#1082250

I’ve submitted a PR on the t3 github repo:

https://github.com/t3framework/t3/pull/515

[Saguaros Moderator SaguarosJoin date: September 2014Posts: 31405Downloads:237Uploads:471 Thanks: 845 Thanked: 5346 times in 4964 posts]

#1082319

Hi Hugh,

Thanks for contacting us!

Would you mind sharing an example where we can replicate the issue of this conflict? I will share with the dev crew for further consideration.

Regards

[cheesegrits Friend cheesegritsJoin date: August 2007Posts: 10Downloads:5Uploads:0 Thanked: 1 times in 1 posts]

#1082544

Thanks for the response.

I don’t have a public server I can point you at right now, but one of my clients (a Fabrik user) should be following up here soon with an example, and I’ll point out the specifics when he does.

— hugh

[burghard Friend burghardJoin date: October 2015Posts: 12Downloads:4Uploads:2]

#1082695

You can find a simple example at my site britzke.berlin. As you can see, the on/off button near "nur Zusammenfassung" is rendered as a blank rectangle.

[cheesegrits Friend cheesegritsJoin date: August 2007Posts: 10Downloads:5Uploads:0 Thanked: 1 times in 1 posts]

#1082763

And as you can see, this is because the fieldset is rendered like this:

<fieldset class="radio btn-radio t3onoff" data-toggle="buttons" style="padding-top:0px!important">
<label for="fab_newsletter___nur_zusammenfassung_input_0" class="fabrikgrid_0  btn-default on">
    <input type="radio" class="fabrikinput " name="fab_newsletter___nur_zusammenfassung[]" id="fab_newsletter___nur_zusammenfassung_input_0" value="0"><span>Nein</span>
</label>

<label for="fab_newsletter___nur_zusammenfassung_input_1" class="fabrikgrid_1  btn-default on">
    <input type="radio" class="fabrikinput " name="fab_newsletter___nur_zusammenfassung[]" id="fab_newsletter___nur_zusammenfassung_input_1" value="1" checked="checked"><span>Ja</span>
</label>
</fieldset>

… with the radio input inside the label, rather then preceeding it. The t3onoff code as written assumes that the input will preceed the label. Which it does in native J! forms, but that’s just a layout convention.

This is easily remedied by just using the ‘for’ property of the label to find the input, rather than looking for prev(), as per my PR.

Hugh

[Saguaros Moderator SaguarosJoin date: September 2014Posts: 31405Downloads:237Uploads:471 Thanks: 845 Thanked: 5346 times in 4964 posts]

#1082883

Hi guys,

Thanks for sharing, let me share with the dev crew for further consideration.

Regards

[cheesegrits Friend cheesegritsJoin date: August 2007Posts: 10Downloads:5Uploads:0 Thanked: 1 times in 1 posts]

#1082965

Thanks. And just a reminder, there’s a pull request here:

https://github.com/t3framework/t3/pull/515

… with the fix.

1 user says Thank You to cheesegrits for this useful post

1. Saguaros

[sunnyjey Friend sunnyjeyJoin date: March 2007Posts: 36Downloads:25Uploads:5 Thanks: 8 Thanked: 1 times in 1 posts]

#1087531

Thank you Hugh for the fix. Love you.

• This reply was modified 6 years, 11 months ago by  sunnyjey.

1 user says Thank You to sunnyjey for this useful post

1. Saguaros

[burghard Friend burghardJoin date: October 2015Posts: 12Downloads:4Uploads:2]

#1089917

Will the commits from the above pull request find their way into the master branch? I saw it is still open.

[Saguaros Moderator SaguarosJoin date: September 2014Posts: 31405Downloads:237Uploads:471 Thanks: 845 Thanked: 5346 times in 4964 posts]

#1090022

Yes, it will be merged soon.

[Author]

Posts