Improvements in layouts usage in fields by phproberto · Pull Request #8248 · joomla/joomla-cms

phproberto · 2015-11-02T17:10:45Z

Index

Improvements
1.1. [imp] Convert JFormField::getInputLayoutData() into JFormField::getLayoutData()
1.2. [imp] Add a base JFormField::getInput() method that can be inherited
1.3. [imp] Allow to debug fields rendering
1.4. [imp] Add support to specify the rendering layout in form's XML
Additional testing information

1. Improvements

1.1. Convert JFormField::getInputLayoutData() into JFormField::getLayoutData()

I think the purpose of that method is that we had different methods to get layout data for the label and for the input of the fields but we only need a single getLayoutData() that can be passed to getLabel() and getInput(). Sometimes frontenders face an issue that involves rendering the label of the field in the field itself. And that's easier if you have everywhere the information required to render anything.

1.2. Add a base JFormField::getInput() method that can be inherited

Now all the fields that extend JFormField can inherit the getInput() method and mostly remove it. Actually that should be our priority because getInput() method is the bigger headache for frontenders now. Removing it clearly sets a line betwee what backenders need to do (populate the required data in the getLayoutData() method) and frontenders need to do (style that data in the layouts).

Note that currently all the fields override the getInput() method so at current state only direct childs of JFormField will inherit getInput(). A good example is JFormFieldCheckboxes that inhertis JFormFieldList. So until the getInput() mehotd of JFormFieldList is not moved to a layout JFormFieldCheckboxes has to keep his own override.

1.3. Allow to debug fields rendering

Now fields that use the new JLayoutFile renderer will allow that developers enable/disable debug mode in the form XML just adding debug="true".

Example:

<field name="lineNumbers" type="radio"
       class="btn-group btn-group-yesno"
       debug="true"
       default="0"
       filter="options"
       description="PLG_CODEMIRROR_FIELD_LINENUMBERS_DESC"
       label="PLG_CODEMIRROR_FIELD_LINENUMBERS_LABEL"
    >
    <option value="1">JON</option>
    <option value="0">JOFF</option>
</field>

When the form is rendered it will show something like:

1.4. Add support to specify the rendering layout in form's XML

Now fields also support that the same field can be rendered using different layouts just adding layout="mylibrary.field.name". That increases fields reusability because you can, for example, use a text field to add a currency value, an AJAX field, etc.

Example:

<field name="lineNumbers" type="radio"
       class="btn-group btn-group-yesno"
       default="0"
       layout="mylibrary.field.radio"
       filter="options"
       description="PLG_CODEMIRROR_FIELD_LINENUMBERS_DESC"
       label="PLG_CODEMIRROR_FIELD_LINENUMBERS_LABEL"
    >
    <option value="1">JON</option>
    <option value="0">JOFF</option>
</field>

3. Additional testing information

This PR changes internal behavior of fields. So:

Try to edit most common used forms.
Particularly check forms that involve type="radio" and type="checkboxes" fields because they have received most of the modifications.

wilsonge · 2015-11-02T17:13:25Z

libraries/joomla/form/field.php

we should throw an exception if the layout is null right for b/c?

to be honest it is confuse 😄
I think if element has layout attribute, then it should go as other attributes through the setup() method, and then also available in __get() __set() .. so can be changed/used at runtime easily as $field->layout = 'bla.bla' 😉

dgrammatiko · 2015-11-02T20:09:32Z

I have tested this item ✅ successfully on ff2f830

Core front end, back end, 3pd components all ok!
Updated the 3 fields PRs to get in-line with this

Thanks @phproberto

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248.}

phproberto · 2015-11-02T20:31:43Z

@DGT41 note that tests fail :D I'll fix it in a couple of hours

phproberto · 2015-11-02T22:20:16Z

I have removed the commit that removes the JLayout HTML comments from here. I will submit a new PR so we can merge it for v3.5

phproberto · 2015-11-02T22:58:09Z

Now getInput throws an exception when there is not active layout. Thanks @wilsonge.
Now layout is processed in field setup and it can be accessed/modified with magic methods. Thanks @Fedik

Now let's try to get those tests working :P

phproberto · 2015-11-02T23:48:54Z

Tests passed now 💃

dgrammatiko · 2015-11-03T06:57:11Z

libraries/joomla/form/field.php

Why classes and not class? Ok I get that there might be many classes for the attribute, so in essence plural it is correct, but the attribute itself is singular class="whatever whatever_more". I mean in the layout I would expect to write something like this $this->class

I think it because here is the array of classes, same for labelClasses 😉

@Fedik I know but we still can declare class in the array and then use $this->class in the layout. It is not a problem or a bug, it’s only that all the other vars are exactly the same as the attributes they are meant for. Basically a naming thing 😃

I agree and I use class for my own but classes was already there so I kept it for B/C just in case anybody has already an override for checkboxes or radio fields. I also used labelClasses to be congruent with the solution that already existed.

Maybe I can add both? Not sure if that would be confusing. I can also rename the existing layouts to:

joomla/form/field/radio.php
joomla/form/field/checkboxes.php

Note the singular (field). That's the path I use for my own and renaming the layout will allow that old systems still work. Any old layout that already exist won't be loaded but the fields will work.

Thoughts?

Although this is a really minor thing, I can see someone in the far future facepalming himself trying to understand why these two vars are plural instead of singular.
As far as I can tell: I don’t like the idea of having both class and classes in the array, but for changing the fields I guess we need a PLT decision. So I guess we are stuck once again with another weird convention 😃

I vote for moving layouts. It will use the same structure than fields and I don't think there is a lot of people with overrides now.

What do you mean with this last comment Roberto? Not sure I understand

Move layouts from:
layouts/joomla/form/fields/*
To:
layout/joomla/form/field

That way we can be sure nobody has an override and we can change the layout vars.

We can't move the ones that are already there - unless you can come up with a b/c fix. But you can move the form fields that we have added for 3.5

joomla-cms-bot · 2015-11-03T23:39:51Z

This PR has received new commits.

CC: @DGT41

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248.}

phproberto · 2015-11-03T23:41:46Z

Ok. This is what I did:

JFormField will pass class & labelclass to layouts
To keep B/C in radio & checkboxes getLayoutData() methods I added the classes value for their layouts

I think this is the best we can do now.

wilsonge · 2015-11-03T23:44:45Z

If the layouts are new we can get away with it. Let me just check but I think they are new for 3.5 - if so then we can get away with it I guess

dgrammatiko · 2015-11-03T23:46:12Z

Nice now I have to redo the other PRs 😝

wilsonge · 2015-11-03T23:49:35Z

#4022 we merged this october 1st. so no b/c issues with moving the layouts - my bad!

phproberto · 2015-11-03T23:51:51Z

I'm moving layouts to layouts/joomla/form/field then. That doesn't affect you @DGT41 so let's get all together!

phproberto · 2015-11-04T00:23:09Z

yes!

…rmField

joomla-cms-bot · 2015-11-04T00:39:27Z

This PR has received new commits.

CC: @DGT41

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248.}

phproberto · 2015-11-04T00:46:19Z

Is ready for new tests if travis gives the ok.

Being there testing the fields I noticed that codemirror fullScreenMod parameter was not aligned properly so I fixed it (remove 1 line). Just in case somebody see the XML modified.

Before:

After:

zero-24 · 2015-11-04T13:54:37Z

I have tested this item ✅ successfully on 92225ca

Works good. I have tested this in combination with the other PRs by @DGT41 and can confirm that the radio and checkboxes (codemirror) in Backend still works. Thanks

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248.}

dgrammatiko · 2015-11-04T15:00:31Z

I have tested this item ✅ successfully on 92225ca

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248.}

zero-24 · 2015-11-04T15:06:52Z

RTC. Thanks.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248.}

Improvements in layouts usage in fields

wilsonge · 2015-11-04T15:07:54Z

Merged - thanks guys 😃

zero-24 · 2015-11-04T15:08:44Z

A hug thank you goes to @phproberto 👍 That is great stuff!

phproberto · 2015-11-04T15:19:34Z

Thanks to everybody involved in testing, commenting, improving and merging 💃

roland-d · 2015-11-04T15:23:09Z

Thank you @phproberto for your contribution.

joomla-cms-bot added the PR-staging label Nov 2, 2015

wilsonge reviewed Nov 2, 2015
View reviewed changes

phproberto force-pushed the jformfield branch from cb7927a to ff2f830 Compare November 2, 2015 17:57

phproberto force-pushed the jformfield branch from ff2f830 to ab29622 Compare November 2, 2015 22:55

dgrammatiko reviewed Nov 3, 2015
View reviewed changes

dgrammatiko mentioned this pull request Nov 3, 2015

Move form field moduleposition to layouts and use Bootstrap modal #7702

Closed

zero-24 mentioned this pull request Nov 3, 2015

Versions using layouts and bootstrap #5654

Merged

zero-24 added this to the Joomla! 3.5.0 milestone Nov 3, 2015

zero-24 mentioned this pull request Nov 3, 2015

Move form field user to layouts and use Bootstrap modal #5655

Merged

dgrammatiko mentioned this pull request Nov 3, 2015

Move Media field to layouts and use Bootstrap modal and popover #5871

Merged

phproberto added 4 commits November 4, 2015 00:29

[imp] improvements in layouts usage in fields

df697db

[imp] setup JFormField layout during field setup

4ece524

[imp] throw exception if a form field has not layout assigned

7432741

[fix] JFormField cannot inherit JFormFieldList getInput

a27627c

phproberto force-pushed the jformfield branch from c837a03 to 2c90116 Compare November 3, 2015 23:39

phproberto added 3 commits November 4, 2015 01:24

[imp] keep classes in checkboxes & radio layouts but use class in JFo…

f1b669f

…rmField

[imp] move radio layout + improve information

2a46efe

[imp] move checkboxes field layout + improve information

51204f5

[imp] codemirror plugin fullScreenMod option alignment

92225ca

phproberto force-pushed the jformfield branch from 2c90116 to 92225ca Compare November 4, 2015 00:39

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 4, 2015

wilsonge added a commit that referenced this pull request Nov 4, 2015

Merge pull request #8248 from phproberto/jformfield

922102d

Improvements in layouts usage in fields

wilsonge merged commit 922102d into joomla:staging Nov 4, 2015

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 4, 2015

phproberto deleted the jformfield branch November 4, 2015 15:19

Uh oh!

Conversation

phproberto commented Nov 2, 2015

Index

3. Additional testing information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgrammatiko commented Nov 2, 2015

Uh oh!

phproberto commented Nov 2, 2015

Uh oh!

phproberto commented Nov 2, 2015

Uh oh!

phproberto commented Nov 2, 2015

Uh oh!

phproberto commented Nov 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joomla-cms-bot commented Nov 3, 2015

Uh oh!

phproberto commented Nov 3, 2015

Uh oh!

wilsonge commented Nov 3, 2015

Uh oh!

dgrammatiko commented Nov 3, 2015

Uh oh!

wilsonge commented Nov 3, 2015

Uh oh!

phproberto commented Nov 3, 2015

Uh oh!

phproberto commented Nov 4, 2015

Uh oh!

joomla-cms-bot commented Nov 4, 2015

Uh oh!

phproberto commented Nov 4, 2015

Uh oh!

zero-24 commented Nov 4, 2015

Uh oh!

dgrammatiko commented Nov 4, 2015

Uh oh!

zero-24 commented Nov 4, 2015

Uh oh!

wilsonge commented Nov 4, 2015

Uh oh!

zero-24 commented Nov 4, 2015

Uh oh!

phproberto commented Nov 4, 2015

Uh oh!

roland-d commented Nov 4, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants