-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements in layouts usage in fields #8248
Conversation
abstract protected function getInput(); | ||
protected function getInput() | ||
{ | ||
$layout = $this->getAttribute('layout', $this->layout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should throw an exception if the layout is null right for b/c?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
😉
cb7927a
to
ff2f830
Compare
I have tested this item ✅ successfully on ff2f830 Thanks @phproberto This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248. |
@dgt41 note that tests fail :D I'll fix it in a couple of hours |
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 |
ff2f830
to
ab29622
Compare
Tests passed now 💃 |
'validate' => $this->validate, | ||
'value' => $this->value | ||
'autofocus' => $this->autofocus, | ||
'classes' => explode(' ', $this->class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it because here is the array of classes, same for labelClasses 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with this last comment Roberto? Not sure I understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
c837a03
to
2c90116
Compare
This PR has received new commits. CC: @dgt41 This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248. |
Ok. This is what I did:
I think this is the best we can do now. |
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 |
Nice now I have to redo the other PRs 😝 |
#4022 we merged this october 1st. so no b/c issues with moving the layouts - my bad! |
I'm moving layouts to layouts/joomla/form/field then. That doesn't affect you @dgt41 so let's get all together! |
yes! |
2c90116
to
92225ca
Compare
This PR has received new commits. CC: @dgt41 This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248. |
I have tested this item ✅ successfully on 92225ca This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248. |
I have tested this item ✅ successfully on 92225ca This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248. |
RTC. Thanks. This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8248. |
Improvements in layouts usage in fields
Merged - thanks guys 😃 |
A hug thank you goes to @phproberto 👍 That is great stuff! |
Thanks to everybody involved in testing, commenting, improving and merging 💃 |
Thank you @phproberto for your contribution. |
Index
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
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 togetLabel()
andgetInput()
. 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 thegetInput()
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 ofJFormField
will inheritgetInput()
. A good example isJFormFieldCheckboxes
that inhertisJFormFieldList
. So until thegetInput()
mehotd ofJFormFieldList
is not moved to a layoutJFormFieldCheckboxes
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:
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:
3. Additional testing information
This PR changes internal behavior of fields. So: