-
-
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
New field Subform, and deprecate the Repeatable field #7829
Conversation
|
||
foreach (array('formsource', 'min', 'max', 'layout', 'groupByFieldset', 'buttons') as $attributeName) | ||
{ | ||
$this->__set($attributeName, $element[$attributeName]); |
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 are you calling the magic method directly here instead of $this->$attributeName = $element[$attributeName];
?
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.
some of the values need to "check" before "set" ... so I do all in one place (see __set
method), same as in JFormField 😉
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.
Something screams majorly wrong if we are having to call magic methods ourselves in the code...
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.
and because calling $this->$attributeName
will skip the magic __set
... in current scope
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.
🍺 API Fail 🍺
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 with @mbabker we shouldn't be calling magic methods ourselves. It doesn't sound right for sure, we should have a proper method handling this, maybe even getters/setters for private fields.
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.
it is out of scope for the current issue,
but as alternative solution, we can use magic __call
instead of magic __get
__set
then code will looks like $field->{'set' . $attributeName}($value)
that will work same inside the class : $this->{'set' . $attributeName}($value)
or $this->setSomeProperty($value)
.. same for get
...
but this is not b/c and I not sure that it is a better solution, because need to handle both Get and Set inside one method 😄
|
</div> | ||
<?php endif; ?> | ||
|
||
<?php foreach($form->getGroup('') as $field): ?> |
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.
hmm not sure is this correct?
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.
yes it is,
it for take all available fields, see https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/form.php#L1680-L1684
@Fedik This PR looks very promising. Will it be helpful to load separate setting for different module layouts? In your solution formsource="path/to/form.xml" is set strictly. If the answers to both questions are positive, that would be really great. |
@shur answer is No, if I right understand your question or you can make a couple subform fields with different forms, and switch them using |
@test |
I want to note: current pull request do not fix that issue #6882 (for repeatable mode). @dgt41 that will be hard 😨 ... but you are right, will be bad if it will be broken, again 👽 ... and thanks for link that issue. Main problem with that Media field is "hard" linking to the input "id". About Chosen script, the problem that we do not use "special" class where we want to have the Chosen script, and just apply that script for all |
@dgt41 sorry man, no chance with new media field, it even more hard than was with Mootools. $row.find('div[id^="media_field_"]').each(function(){
var $inpWrapp = $(this),
$modal = $inpWrapp.siblings('div[id^="imageModal_"]'),
inputId = $inpWrapp.children('input[type="text"]').prop('id'),
$btnSel = $inpWrapp.children('a[href^="#imageModal_"]'),
$btnClr = $inpWrapp.children('a[onclick^="clearMediaInput"]'),
$preview = $inpWrapp.children('span[id^="media_preview_"]');
$inpWrapp.attr('id', 'media_field_' + inputId); // fix wrapper ID
$modal.attr('id', 'imageModal_' + inputId); // fix modal ID
$preview.attr('id', 'media_preview_' + inputId); // fix preview ID
$btnSel.attr('href', '#imageModal_' + inputId); // fix Select button
// fix clear btn
var onclic = $btnClr.attr('onclick').replace(/clearMediaInput\('[0-9a-zA-Z\_\-]+', /, "clearMediaInput('" + inputId + "', ");
$btnClr.attr('onclick', onclic);
}); This fix already looks like trash, and it just a 10% of fix that need to do for new Media field from #5871. |
There should be an easier way to utilize those two functions in the js file... |
@dgt41 any tips are welcome 😉 |
It was a problem in staging - I just fixed it - if you rebase/merge in staging should be ok |
I have tested this item ✅ successfully on 0e8f6f7 RTC This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7829. |
Any suggestion are welcome. |
@Fedik if you fix the Code Style issues we can merge this :-) |
@rdeutz restart travis |
Merged - travis failure was unrelated and was due to failures we had in staging in manchester. |
I just now noticed, I have missed the year, fix in #10375 |
That's really awesome and working very well!! However I have noticed two things:
|
@WS-Theme This is a closed / merged issue. Please open a new one at https://issues.joomla.org else i guess your findings get lost. Thanks |
@zero-24 sorry my error! Opened a Issue @ Joomla |
Great. Thanks 😄 |
Please create a new issue. No one will see a comment on a closed issue
|
@brianteeman sorry. Done! |
no need new issue, just use correct layout |
Notice:
from Fedik at #11551 : |
Joomla! Documentation https://docs.joomla.org/Subform_form_field_type
This is the field I have suggested in #7749
Description
The field allow to include any existing form into the current form. If attribute
multiple
set totrue
then the included form will be repeatable. The Field have a couple "predefined" layouts for display the subform astable
or asdiv
container, and of course it allow to use your own layout.Field support Default values from the included form, and from
json
string indefault
attribute. Last have higher priority.Example:
Attributes:
formsource
- (required) The form source to be included. Path to xml file or the form name to search byJForm::getInstance()
.multiple
- The multiple state for the form field. Whether the field is repeatable or not.min
- Count of minimum repeating in multiple mode. Default 0.max
- Count of maximum repeating in multiple mode. Default 1000.groupByFieldset
- Whether group the subform fields by it`s fieldset (true or false).buttons
- Which buttons to show in multiple mode. Defaultadd,remove,move
.layout
- The layout name for render the field inputs. Available:joomla.form.field.subform.default
- Render the subform indiv
container, without support of repeating.joomla.form.field.subform.repeatable
- Render the subform indiv
container, recommended formultiple
mode. SupportgroupByFieldset
.joomla.form.field.subform.repeatable-table
- Render the subform astable
, recommended formultiple
mode. SupportgroupByFieldset
. By default render each field as the table column, but ifgroupByFieldset=true
then render each fieldset as the table column.Testing
Can use "Custom HTML" module. Add the field into the module xml:
Create the form xml in
modules/mod_custom/test1.xml
. Example:And of course you can use your own form.
Now go to edit the "Custom HTML" options and try add/remove the values for new field.![:neckbeard: :neckbeard:](https://github.githubassets.com/images/icons/emoji/neckbeard.png)
Try play with different field attributes
multiple
,min
,max
,buttons
,layout
,groupByFieldset
and make sure that the field work.p.s. The pull request does not provide the
Validation
andFilter
. But such thing can be implemented in future, at least forValidation
, for theFilter
can be need someJForm
changes.