Viewing 12 posts - 1 through 12 (of 12 total)
  • Author
    Posts
  • cheesegrits JATC
    #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 JATC
    #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 JATC
    #1082250

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

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

    Saguaros Moderator
    #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 JATC
    #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
    #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 JATC
    #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
    #1082883

    Hi guys,

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

    Regards

    cheesegrits JATC
    #1082965

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

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

    … with the fix.

    sunnyjey Friend
    #1087531

    Thank you Hugh for the fix. Love you.

    • This reply was modified 6 months, 3 weeks ago by  sunnyjey.
    burghard Friend
    #1089917

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

    Saguaros Moderator
    #1090022

    Yes, it will be merged soon.

Viewing 12 posts - 1 through 12 (of 12 total)

This topic contains 11 replies, has 4 voices, and was last updated by  Saguaros 6 months, 1 week ago.

We moved to new unified forum. Please post all new support queries in our New Forum