User tests: Successful: Unsuccessful:
Same as #2511 but to staging instead of master
For this tracker item: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32695&start=0
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.
@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
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
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.
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
JLayout is part of the cms library. Isn't it inappropriate to use it in fields which are part of the joomla library?
@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.
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?
@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
Labels |
Added:
?
?
|
JLayout is pretty cool.
Oh snap, I forgot about the tests!
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.
@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.
@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.
I see your point. I guess I'll do it that way then. But I can't do anything until tomorrow morning.
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
@studio42 By the way: we are not only talking about backend templates here.
@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
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
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.
@okonomiyaki3000 Very nice idea to convert formfieds render to JLayout..
I fight all the time with the rendering, because fomrfields cannot be customised .
@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.
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.
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
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.
@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 ;)
Hmm. I think I shall rewrite some parts of the layouts to remove any dependence on $this
being passed from the field.
Title |
|
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.
@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.
Hmm. Looks like a test is wrong.
@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.
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.
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.
So.. anything happening here?
As far as I see, no successful tests are logged yet in the JC item. Without tests, nothing will happen.
Rebased to fix merge conflict.
Title |
|
Hey, look! It still passes. So are layouts a priority at all?
@phproberto Maybe something for the frontenders to look at? Or maybe even duplicate work?
Closed as per the comment on the tracker
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-07-27 16:37:57 |
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.