? ? ? Success

User tests: Successful: Unsuccessful:

avatar JoomliC
JoomliC
9 Jun 2016

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.

Benefits of this new form field type

  1. BS modal without the need of an override in the most used admin template (Isis)
  2. Use JLayout for form field, and allow user to override it.
  3. B/C with same xml attribute type="user"
  4. New optional xml attribute basetype="modal" to define the prefix/path and load the new form field Modal_User
  5. The new 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 ;-) )

Summary of Changes

capture d ecran 2016-06-09 a 20 53 44

capture d ecran 2016-06-10 a 00 37 43

  • Introduces new attribute 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
  • if "basetype" is not defined (eg. third party extension) will load jFormFieldUser, if defined, will load jFormField[basetype]_user
  • Removal of layout form field user override from Isis Template (added only in Hathor) and let's user decide to create its own override for this form field
  • NEW Edit button to allow or not edit of a user (xml attribute edit="true")
  • NEW New button to allow or not to create directly in modal a new user (xml attribute new="true")
  • NEW 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!

Testing Instructions

  • Test on 3.6.0-beta or latest staging.
  • All fields type="user" in core xml files are updated in this PR.
  • Test those form fields in edition pages:
    • Banner com_banners (Created By + Modified By (readonly))
    • Category com_categories (Created By + Modified By (readonly))
    • Contact com_contact (User + Created By + Modified By (readonly))
    • Article com_content (Created By + Modified By (readonly))
    • Filter com_finder (Created By + Modified By (readonly))
    • Newsfeed com_newsfeeds (Created By + Modified By (readonly))
    • Tag com_tags (Created By + Modified By (readonly))
    • Note com_users (Created By + Modified By (readonly))
avatar JoomliC JoomliC - open - 9 Jun 2016
avatar JoomliC JoomliC - change - 9 Jun 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 9 Jun 2016
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 9 Jun 2016
Labels Added: ? ?
avatar dgt41
dgt41 - comment - 9 Jun 2016

@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.

avatar JoomliC
JoomliC - comment - 10 Jun 2016

@dgt41 Thanks! Didn't know that... I will see to test repeatable form field, but what is the "javascript part for media and user fields" ?

avatar JoomliC
JoomliC - comment - 10 Jun 2016

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.

avatar andrepereiradasilva
andrepereiradasilva - comment - 10 Jun 2016

wow nice job!!

avatar brianteeman brianteeman - change - 10 Jun 2016
Category Fields UI/UX
avatar brianteeman brianteeman - change - 10 Jun 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 10 Jun 2016
Labels
avatar brianteeman brianteeman - change - 10 Jun 2016
Category Fields UI/UX Fields Language & Strings Layout UI/UX
avatar brianteeman brianteeman - change - 10 Jun 2016
Labels
avatar JoomliC
JoomliC - comment - 10 Jun 2016

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... :-(

avatar Fedik
Fedik - comment - 10 Jun 2016

@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.

avatar JoomliC
JoomliC - comment - 10 Jun 2016

@Fedik Do you have tested this PR with repeatable ?
As, when testing:
First field :

  • id for BS modal selector = jform_params__field_name__field_name0__test_modal_user
  • id for hidden input (user ID) = 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:

  • 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

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 ?)

avatar dgt41
dgt41 - comment - 10 Jun 2016

@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.

avatar Fedik
Fedik - comment - 11 Jun 2016

@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.

avatar dgt41
dgt41 - comment - 11 Jun 2016

@JoomliC take a look at #10788

avatar JoomliC
JoomliC - comment - 11 Jun 2016

@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 ?

avatar JoomliC
JoomliC - comment - 11 Jun 2016

@fedik There's an issue with repeatable subform!
Just test with exactly the form field here: #7829

What i did :

  • Select a tag for row 1
  • Add a new row
  • Select a tag for row 2
  • Add a new row
  • Select a tag for row 3
  • ...
  • Save

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... :-|

avatar Fedik
Fedik - comment - 11 Jun 2016

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. ????

avatar JoomliC
JoomliC - comment - 11 Jun 2016

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....) ?

avatar Fedik
Fedik - comment - 11 Jun 2016

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 ????

avatar JoomliC
JoomliC - comment - 11 Jun 2016

@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! )

avatar Fedik
Fedik - comment - 11 Jun 2016

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.

avatar JoomliC
JoomliC - comment - 12 Jun 2016

@Fedik Thank you for ideas! ????
Will check when a bit of more time, what could be done (i'm sure there's a way ;-) )

avatar zero-24
zero-24 - comment - 5 Oct 2016

@JoomliC any update here or just no time? As this also has conflicts

avatar JoomliC
JoomliC - comment - 7 Oct 2016

@zero-24 ? Thank you to remind me to close this PR, where sadly i have lost time and energy.
Closed!

avatar JoomliC JoomliC - change - 7 Oct 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-10-07 00:27:37
Closed_By JoomliC
avatar JoomliC JoomliC - close - 7 Oct 2016

Add a Comment

Login with GitHub to post a comment