User tests: Successful: Unsuccessful:
This PR introduces a new form field Modal_User
.
Current formfield user
is using mootools modal, and since 3.5, it uses a layout override in Isis template to use Bootstrap modal (with a few B/C issues reported).
IMO, not good to manage core improvement in template overrides... as it does not let user manage its own override.
type="user"
basetype="modal"
to define the prefix/path and load the new form field Modal_User
basetype
attribute gives facilities for future other changes.If it's ok for this PR concerning the user modal, i can do the same process for media
form field! ;-)
cc/ @dgt41 @wilsonge : i need your review and comment here, as you know well about the issue with B/C when trying to use Bootstrap modals for user and media form fields ! Thanks! ;-)
Note: i can do this ASAP for Media too, for 3.6.0-beta2, if it's ok with this one about User
Bonus idea: i can migrate category and article modals to use layouts and fields in library for re-usuability (i don't think it should be done for contact, newsfeed... as not directly the base core components).
cc/ @andrepereiradasilva as you will see, i've moved the "no-user" button from the modal content, to the form field (and in addition, a "New" button added to create an new item directly inside a modal. A new feature i will see to add to category, article, and other edit modals ;-) )
basetype
to define the path of the form field. eg. "modal" if in path (/libraries/joomla/form/fields/ OR [component]/models/fields/) modal/user.php[basetype]
_userEdit
button to allow or not edit of a user (xml attribute edit="true"
)New
button to allow or not to create directly in modal a new user (xml attribute new="true"
)No User
button to allow or not to left field input empty (xml attribute clear="true"
or if required required="true"
)All questions and feedback are welcome!
type="user"
in core xml files are updated in this PR.com_banners
(Created By + Modified By (readonly))com_categories
(Created By + Modified By (readonly))com_contact
(User + Created By + Modified By (readonly))com_content
(Created By + Modified By (readonly))com_finder
(Created By + Modified By (readonly))com_newsfeeds
(Created By + Modified By (readonly))com_tags
(Created By + Modified By (readonly))com_users
(Created By + Modified By (readonly))Status | New | ⇒ | Pending |
Labels |
Added:
?
?
|
Labels |
Added:
?
?
|
Thanks!
Just tested, saw issue, and see why the script fielduser.js, i thought only for mootools... (and then the one for media...)
i will check this tomorrow, and see if not possible to do something more simple and easy to maintain... just to work with repeatable.
wow nice job!!
Category | ⇒ | Fields UI/UX |
Labels |
Added:
?
|
Labels |
Category | Fields UI/UX | ⇒ | Fields Language & Strings Layout UI/UX |
Labels |
There might be a slight problem with this approach (thats why me and @fedik worked on the javascript part for media and user fields) and that is: it won;t (? haven;t tested, obviously) work with the new repeatable form field.
@dgt41 cc/ @Fedik
Unfortunately, i see some issues with the way repeatable subform script works, which may give issues in the long term...
Eg. : https://github.com/joomla/joomla-cms/blob/staging/media/system/js/subform-repeatable-uncompressed.js#L241 (already not updated with the latest version of this external library minicolors.js)
I wonder if we're not going to knock our heads against walls in the future, each time we will update a form field using script...
Isn't this possible to manage repeatable form fields without touching the field's script, and having each time to write a complex js file (a global script, not linked to a specific form field script/library, and so not having one js file for each form field each time only for usage in repeatable context?)
I think it difficult to maintain, even more for an open source project... :-|
I can see to work on a workaround special case for repeatable field, but i don't like the idea: having to write code to be able to work in another form field (subform), and to know that this will be the case for any current and possible upcoming form field type using script.
The main issue in subform script is that each time we will change a script or update an external library used in a form field, we will have to update the subform script too... :-(
@JoomliC the problem that Joomla does not have API to work with dynamic content...
$.subformRepeatable.prototype.fixScripts
just work around, of course it is a wrong way to do the thing, but for now only possibility to make something work
Couple note about your pull:
I would recommend do not use field ID for interact with field. The problem of such approach starts when content dynamically changes. In result you cannot find correct field.
@Fedik Do you have tested this PR with repeatable ?
As, when testing:
First field :
jform_params__field_name__field_name0__test_modal_user
jform_params__field_name__field_name0__test_modal_user
The same, so this one works
Then, i add a new section, and for the second field:
jform_params__field_name__field_nameX__test_modal_user
jform_params__field_name__field_name2__test_modal_user
There, both 2 ones are wrong, as it should be: jform_params__field_name__field_name1__test_modal_user
Maybe the issue is when in process, the id is incremented ? (maybe should be incremented before the field is rendered ?)
@JoomliC will be good if you can use the scripts mediafield.js and userfield.js as they already utilise data attributes and there is 0 lines of inline script code!. The problem, and that was the cause that the bootstrap versions of those two fields didn't reach production 3.5, was an incompatibility with the function jSelectUser (or something similar). Patching that then we can have bootstrap modals and the repeatable working as it was originally designed, the regression was here: #9659. So if you take the layouts/fieelds/user.php form #5655 and place it in isis and protostar you get bootstrapped modal.
@JoomliC I not tested with Repeatable , because I see without test that it will not work with Repeatable script
Here a couple piece of code that hardly linked to field ID:
function jSelectUser_' . $id . '(id, name) {
and
function jEditUser(inputid, btn) {
and function jClearUser(id)
has:
jQuery("#userEdit' . $id . 'Modal").modal("hide");
all this will fail when someone try to add one more field instance on the page, dynamically.
The idea of mediafield.js
and userfield.js
that each field has linked Instance of fieldMedia
/fieldUser
prototype and so you always can access to the correct field without knowing it's ID.
I agree that would be cool to have something one like FieldModal.js
(mediafield.js
and userfield.js
very close in the code) that could share API for build modal fields, but it is a complex task.
@Fedik Using the field id as BS modal selector was already the way for all modals before this PR, and this is not new ;-)
In the same time, we need to have always a unique DOM selector for BS modal, in case we have multiple one on the same page. Field ID seems the easier way to manage this.
About repeatable, i have check a bit more the code, and think there's a possible issue : incrementation of fields (id and name).
In the same time, if you use the function from php (getInput) to generate the form, all is good. This make me wonder if we could maybe solve all the issue we have by using ajax to add a new row, and not purely dynamic script. This way, the incremented ID is well set before each form field is rendered...
Not yet give it a try, but i think we can do something like this, and have no more issue with form fields using script, and no need anymore of fielduser.js not mediauser.js
@dgt41 & @Fedik i can give a try to integrate in this PR what you have already done (fielduser.js and data-*) but i see this solution as a temporary one (that means i would like to find a solution to use ajax for repeatable subform ;-) )
What's your opinion ?
@fedik There's an issue with repeatable subform!
Just test with exactly the form field here: #7829
What i did :
Then check params saved in database :
field-name0
field-name2
field-name3
Then, just save again the module (no need to change anything, but same issue if changing values)
Check back database :
field-name0
is still field-name0
so OK!
field-name2
is now field-name1
so an issue, as name should not be changed...
field-name3
is now field-name2
so issue... and so on...
This confirms the fact id is not well incremented... In fact, the php code works well, not the script... :-|
Using the field id as BS modal selector was already the way for all modals before this PR, and this is not new ;-)
yes, I know, but it does not means it is right. Using things like function jSelectUser_' . $id . '(id, name) {
is wrong (mildly speaking)
Just another one legacy that we should trash.
We should stop to generate JavaScript by PHP.
In the same time, we need to have always a unique DOM selector for BS modal
I agree that ID should be unique but it do not required to have any ID. See $.fieldUser.prototype.modalOpen/modalClose
in fielduser.js
- no ID in use
Then check params saved in database : field-name0 field-name2 field-name3
I seen this behavior, but it doesn't matter. If you change ordering it can be:
field-name3
field-name0
field-name2
And after save the form and open again then it become:
field-name0
field-name1
field-name2
Because https://github.com/joomla/joomla-cms/pull/7829/files#diff-bbec1e130dbc3904a847305a6ff41fb8R230
This names do not used and it a "temporary thing".
that means i would like to find a solution to use ajax for repeatable subform
It would be very cool. But I think you will hit the same problem, Joomla do not have API to work with Dynamic content.
I agree that ID should be unique but it do not required to have any ID. See $.fieldUser.prototype.modalOpen/modalClose in fielduser.js - no ID in use
Ok, so i will have some changes to do... ;-)
This names do not used and it a "temporary thing".
Well... i miss maybe something about repeatable subform...
What is its purpose ?
As if you can't get the values for row "X" as this one identifier could change each time you change ordering, how do you manage then the data saved ?...
For example, if i want to get the value of "test_text" in the saved row "field-name2" (first module save) but after this one will be an other one...
In fact, maybe i don't understand how to get data individually (as it seems that names are only for ordering....) ?
Well... i miss maybe something about repeatable subform...
What is its purpose ?
Complicated to explain
it just for calculate the field NAME and ID for new field, at start it was just a Unix time, like
jform[fieldName][1234561546][subfield]
, later I changed it to
jform[fieldName][fieldNameX][subfield]
as found it more clear.
On start there also was some functionality for keep exact iteration, even if ordering changes it was:
field-name0 field-name1 field-name2. But I remove it 576c15d#diff-2b445aeddc01416d3a59bd4a2e633fbbL91 as it made a loot more problem and was buggy. And had a profit close to zero .
If you need some value manipulation before save the subform field, then I would recomend use:
$subValues = array_values($data['subformField']);
var_dump($subValues[0]); // first Row
var_dump($subValues[1]); // second Row
...
But as I said the iteration does not really matter.
the problem you got:
id for BS modal selector = jform_params__field_name__field_nameX__test_modal_user
id for hidden input (user ID) = jform_params__field_name__field_name2__test_modal_user
Because the Subform script fixes the ID/NAME only for the inputs
, and do not care about other stuff around. That another reason why I keep repeating: do not use ID
@dgt41 and @Fedik Thanks for your feedback, comments and to have opened my mind
I agree that would be cool to have something one like FieldModal.js (mediafield.js and userfield.js very close in the code) that could share API for build modal fields, but it is a complex task.
So, my intention was to add Edit
, New
and Clear
buttons.
What i will do now is to redo this PR (i keep it open until i will update it when will have a better approach ;-) ).
This one will be based on the fielduser.js, and will think it to be usuable with other field : article, contact, newsfeed, category....
So maybe FieldExtended.js
?... or finally FieldModal.js
as could be consistent with the idea to do the same for other modals using a select/edit...
Because the Subform script fixes the ID/NAME only for the inputs, and do not care about other stuff around. That another reason why I keep repeating: do not use ID
????
Yes, you're definitively right, and so i will review this PR ;-)
(and thanks for repeatable explanation! I've got it now! )
for me FieldModal.js is fine
I had some idea before. Maybe I try share it
For make it work in all cases we can use event delegation
.
Some fake code:
<div class="field-modal-wrapper">
<input name="blabla" type="text" class="field-modal-input">
<button type="button" class="field-modal-button" data-action="select" data-url="/link-to-select-view">
Select</button>
<button type="button" class="field-modal-button" data-action="clear">
Clear</button>
<button type="button" class="field-modal-button" data-action="edit" data-url="/link-to-edit-view">
Edit</button>
</div>
// Bind Click to document (delegation),
// so it allow to catch all dinamicaly added inputs
$(documet).on('click', '.field-modal-button', function(event){
var $btn = $(event.target), // Get clicked button
action = $btn.data('action'), // Get requested action
$wrapper = $btn.closest('.field-modal-wrapper'), // Get wrapper
instance = $wrapper.data('fieldModal'); // Check if we alredy have initialized fieldModal prototype
// Init new instance if there no one
if(!instance){
// Prepare options from data
var options = {}, data = $wrapper.data();
// Check options in the element
for (var p in data) {
if (data.hasOwnProperty(p))
options[p] = data[p];
}
// Set up new instance and attach it to the current field element
instance = new fieldModal($wrapper, options);
$wrapper.data('fieldModal', instance);
}
// Call needed action
instance.applyAction(action, $btn.data());
})
// And in fieldModal code
....
....
$.fieldModal.prototype.applyAction = function(action, options){
if(this[action]){
this[action](options);
} else {
throw new Error('Action "' + action + '" not supported.');
}
}
$.fieldModal.prototype.select = function(options){
// Here call open modal with options.url
}
$.fieldModal.prototype.edit = function(options){
// Here call open modal with options.url for edit
}
$.fieldModal.prototype.clear = function(options){
// Here clear the input value
}
...
...
The tricky thing is to handle select in modalOpen
, because it can have diferent markup.
That one of the reason why we made 2 version mediafield.js
and fielduser.js
.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-10-07 00:27:37 |
Closed_By | ⇒ | JoomliC |
@JoomliC Nice work!
There might be a slight problem with this approach (thats why me and @fedik worked on the javascript part for media and user fields) and that is: it won;t (? haven;t tested, obviously) work with the new repeatable form field.