? Success

User tests: Successful: Unsuccessful:

avatar okonomiyaki3000 okonomiyaki3000 - open - 29 Nov 2013
avatar studio42
studio42 - comment - 30 Nov 2013

Your code is only for bootstrap, this mean other admin templates(eg. hathor) fails to render correctly the checkboxes.
I think the day joomla use layout for formfield, your idea is great but not currently.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 30 Nov 2013

Wrong. The current code fails miserably in bootstrap (which is used by the default template) and is difficult to style. The new code works properly in bootstrap but is also easy to style even if used with a nonbootstrap template.

Hathor can be easily updated to display these fields properly. Bootstrap-based templates will always have a hard time with the older ones.

avatar Bakual
Bakual - comment - 1 Dec 2013

@okonomiyaki3000 I agree with @studio42 here. While checkboxes don't look very well in Isis and Protostar, your patch causes major breakage for non-bootstrapped templates like beez and hathor. So that's probably not going to work out well.
The proper way to do that is likely using JLayouts. Have the default output using HTML standards and then you can add a bootstrapped override to your template. Problem solved without breaking any existing templates :smiley:

avatar studio42
studio42 - comment - 4 Dec 2013

Why not transforming your code to jlayout ? and add the right jlayout file for hathor.
Only an idea ... but i think this is the right solution

avatar okonomiyaki3000
okonomiyaki3000 - comment - 4 Dec 2013

Hmm. Yeah, it looks like that might be the right way to go. But I object to the idea that this code is for Bootstrap. It's not Bootstrap that makes this code work, it's others that make it fail.

avatar Bakual
Bakual - comment - 4 Dec 2013

Actually, you're putting the <input> element inside the <label> element. This isn't wrong but we usually don't use labeled control syntax.
Also you're removing the class from the input and move it to the (now) parent label. This alone will break many templates which have specific styling and maybe JavaScript code which acts on input classes. The id is also removed which again may break styling and JS code.

So while your markup is valid, it just possibly creates a lot of issues with current templates. It's true that this is not strictly a Bootstrap thing, however it breaks the styling for most non-Bootstrap templates.
So please have a look how it could be solved using JLayouts. Then this will be a no-brainer imho :smiley:

avatar okonomiyaki3000
okonomiyaki3000 - comment - 5 Dec 2013

JLayout is part of the cms library. Isn't it inappropriate to use it in fields which are part of the joomla library?

avatar Bakual
Bakual - comment - 5 Dec 2013

@okonomiyaki3000 The joomla library originated from the Joomla platform. Since this no longer exists and is merged back into the cms, this doesn't matter at all. They could be merged fine into one library imho.
Btw, we do that kind of stuff in various places already. Like for example in the calendar formfield where we use JHtml::calendar from the cms library.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 5 Dec 2013

I guess I don't know what's going on since the change from 'platform' to 'framework'. I guess I thought that the cms library was meant to be dependent upon the joomla library but not the other way around. This is not the case, then?

avatar Bakual
Bakual - comment - 5 Dec 2013

@okonomiyaki3000 The platform got merged back into cms. There is no platform anymore. But we kept the libraries named like this for BC reasons.
The framework is a new, so far independant project. Only a few things (like DI) got backported from it and they live in /libraries/framework as far as I know.
https://groups.google.com/forum/#!searchin/joomla-dev-cms/libraries/joomla-dev-cms/0eDCO6L1DJk/r0xGUOlGO18J has some information from Michael about this.

So feel free to use the libraries as it fits :smiley:

avatar okonomiyaki3000 okonomiyaki3000 - change - 6 Dec 2013
Labels Added: ? ?
avatar okonomiyaki3000
okonomiyaki3000 - comment - 6 Dec 2013

JLayout is pretty cool.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 6 Dec 2013

Oh snap, I forgot about the tests!

avatar studio42
studio42 - comment - 9 Dec 2013

Only one think, you don't have to make 2 JLayout fields checkboxes and xheckboxes_bootstrap (same for radio),
but one "standard" in layouts/joomla/fields/checkboxes.php.
and another overrides for hathor in administrator\templates\hathor\html\layouts\joomla\fields\checkboxes.php
(if bootstrap is considerate as "standard" of course)
because another template have to make the same override, if they don't use the "standard" checkboxes and jlayout, is mean to be used so.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 9 Dec 2013

@studio42 Yep, I'm aware of that. The reasons I'm doing it this way are:
A) Bootstrap is not the standard. Maybe it is the future but but, for backwards compatibility with any older templates (those that don't ship with Joomla), the standard should remain unchanged.
B) Because Bootstrap is used by two of the built in templates and likely to be used by many others, a lot of copy/pasting can be saved by including a bootstrap layout. Of course, it is not automatically used but you can write an override that uses it with only one line of code. I've updated Hathor and Protostar to behave this way as an example to anyone who wants to write a template using Bootstrap.

And now I've included updated tests. I'm using the assertTag() test function because it's more appropriate than simple string comparison but also because the strings created by layout files can get pretty complicated due to spacing issues and whatnot. This test is, rightly, unconcerned with such things.

avatar studio42
studio42 - comment - 9 Dec 2013

@okonomiyaki3000, i know you wan't simplify the way to do but hathor have many overrides because bootstrap is considerate as the "standard", i think that in such case, perhaps, the solution is to define what is "standard", but i think it's for bootstrap(as you do at begin) if another admin template use this fields perhaps he don't wan't your code or the second one and override it. You think in this case the template maker must add it on the joomla core ?
In this case the main developpers of joomla have put the code from hathor template in jlayout and never use the overrides.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 9 Dec 2013

I see your point. I guess I'll do it that way then. But I can't do anything until tomorrow morning.

avatar Bakual
Bakual - comment - 9 Dec 2013

The "standard" needs to be what we have currently, and not the "bootstrap" way. Changing the standard would potentially break templates which we can easy avoid in this case. So let's not break it.

Have the default layout be the current way (<label for="element"><element>) and override the layout in Protostar/Isis with a nested <label><element></label>
Then it will not break anything and each template can do what it wants and that's all we need to provide :smile:

@studio42 By the way: we are not only talking about backend templates here.

avatar studio42
studio42 - comment - 9 Dec 2013

@Bakual Of course you are right, but all formfields are currently Bootstrapped.
eg.

            $html[] = '<div class="btn-group">';
            $html[] = '<button type="button" class="btn dropdown-toggle">';
            $html[] = '     <span class="caret"></span>';
            $html[] = '</button>';

for JFormFieldCombo()
and the poorest is't that bootstrap 3 don't use it not always this way
Have a look at http://bootply.com/bootstrap-3-migration-guide

avatar Bakual
Bakual - comment - 9 Dec 2013

I just say that this PR may be rejected if it's breaking templates. However if you do it right, nothing will break. So I would go for the safe route where there should be no harm expected.
There is no reason to intentionally break something when it can be avoided.

There is not really a bootstrap standard in this part. We're talking about using <label for="id"> vs nesting the element within <label>. Bootstrap does this inconsistently across the formfields. Joomla does it consistently so far. So if we are talking about standards, current code wins :smile:

avatar okonomiyaki3000
okonomiyaki3000 - comment - 10 Dec 2013

Well then, let's avoid breaking templates. I believe it currently will not break any. And maybe over time we can move all the fields to a similar system of using JLayout which will give templaters all the flexibility they could need whether they want to use BS 2, 3, 4, or Boilerplate, or anything else.

avatar studio42
studio42 - comment - 10 Dec 2013

@okonomiyaki3000 Very nice idea to convert formfieds render to JLayout..
I fight all the time with the rendering, because fomrfields cannot be customised .

avatar studio42
studio42 - comment - 13 Dec 2013

@okonomiyaki3000 I do it for all formfields thaht need changes in some cases, See https://github.com/studio42/joomla-cms/tree/jlayout-formfields for the current statut. Because i really need it.
Fill free to comment or add your codes to it.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 17 Dec 2013

So, layouts get passed an array with any number of items that might be useful for rendering the thing. Since we're creating new layouts here, I think it's important to decide what gets passed into them because it will be very difficult to change what gets passed in at some later date. So I've added comments to explain the variables I'm using but I'd like any feedback about them. Are they enough? Are they too much? Should they be different in some way? etc.

avatar Bakual
Bakual - comment - 17 Dec 2013

As for what to pass: I would just pass the whole formfield ($this) to the layout. Then the layout has all possible information needed and can act as it wants.
There is no need to create a list of parameters to pass. It's all there in $this :smile:
You can (and should) of course do some preprocessing which likely has to be done in each case, so the layout more or less only has to deal with HTML output. The result of this preprocessing can be attached to $this as well prior to passing it.

avatar studio42
studio42 - comment - 17 Dec 2013

@Bakual $this is not very helpfull, because most of variables and functions are protected.
Another problem is that you don't always come from form fields, but why not your own form generator. If you use this then you have to write it the same way. Suppose you use this in a restfull app or something else not using XML file or a module having 2 or 3 fields in front and want to use jLayout? "This" is not flexible ;)

avatar Bakual
Bakual - comment - 17 Dec 2013

@studio42 Point taken :smiley:

avatar okonomiyaki3000
okonomiyaki3000 - comment - 26 Dec 2013

Hmm. I think I shall rewrite some parts of the layouts to remove any dependence on $this being passed from the field.

avatar okonomiyaki3000 okonomiyaki3000 - change - 27 Dec 2013
Title
boostrap checkboxes
bootstrap checkboxes
avatar studio42
studio42 - comment - 27 Dec 2013

As you can see on my branch, i used "this" because it's simplier to swap the fields to jlayouts, as preconised by the joomla team for my formfield branch.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 30 Dec 2013

@studio42 Didn't you just argue against passing $this to the layout file? I must have misunderstood something.

So, I've added a new function to JFormField which just prepares a $displayData array for the layout file with all the standard elements. Most field types will probably need to extend this and add a few additional elements. This makes it simple to ensure that a standard set of values will get passed to the layout in such a way that a JRegistry can be used. This is pretty convenient for various reasons.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 30 Dec 2013

Hmm. Looks like a test is wrong.

avatar studio42
studio42 - comment - 3 Jan 2014

@okonomiyaki3000 I have do some test and this fails, seams the original joomla code is bugged.
Eg for checkbox you cannot have an empty value, with a default value. But required field have not always to be set to default. and this is the current logic and the value is set to default, but not to value, when you set default !?! Strange, i know how to fix it but i don't know, if this is mean so.
I Hope you understand, english is not my native language.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 3 Jan 2014

@studio42 I know exactly what you mean about saving an empty value when the field has a default value set. Which is why #2617 exists. I don't know why it hasn't been merged yet.

avatar Bakual
Bakual - comment - 3 Jan 2014

have do some test and this fails, seams the original joomla code is bugged. Eg for checkbox you cannot have an empty value, with a default value

I don't think it's a bug in Joomla. It's the checkbox itself which behaves differently in HTML from other form fields. They just don't submit the value if not checked.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 3 Jan 2014

I can understand not wanting to call it a bug but it's certainly something Joomla could handle a little better than it currently does which is what my other PR addresses. In any case, it's not really relevant to this PR.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 23 Jan 2014

So.. anything happening here?

avatar Bakual
Bakual - comment - 23 Jan 2014

As far as I see, no successful tests are logged yet in the JC item. Without tests, nothing will happen.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 18 Apr 2014

Rebased to fix merge conflict.

avatar okonomiyaki3000 okonomiyaki3000 - change - 26 Jun 2014
Title
bootstrap checkboxes
[#32695] bootstrap checkboxes
avatar okonomiyaki3000
okonomiyaki3000 - comment - 4 Jul 2014

Hey, look! It still passes. So are layouts a priority at all?

avatar Bakual
Bakual - comment - 4 Jul 2014

@phproberto Maybe something for the frontenders to look at? Or maybe even duplicate work?

avatar brianteeman
brianteeman - comment - 27 Jul 2014

Closed as per the comment on the tracker

avatar brianteeman brianteeman - change - 27 Jul 2014
Status New Closed
Closed_Date 0000-00-00 00:00:00 2014-07-27 16:37:57
avatar brianteeman brianteeman - close - 27 Jul 2014

Add a Comment

Login with GitHub to post a comment