Skip to content
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

Styling only works for the first repeatable item in Sub Form Fields #11299

Closed
ApolloLV opened this issue Jul 25, 2016 · 28 comments
Closed

Styling only works for the first repeatable item in Sub Form Fields #11299

ApolloLV opened this issue Jul 25, 2016 · 28 comments

Comments

@ApolloLV
Copy link

ApolloLV commented Jul 25, 2016

Steps to reproduce the issue

create a form.xml to display:
`

`

create a subform.xml (use it's file location for the formsource in the form.xml):
`

basic communication skills (A1/A2) good command (B1) very good command (B2) highly proficient (C1) near native (C2) native `

Expected result

All repetitions of the sub form field should be styled the same.

Actual result

subform-bug

Only the first item is styled correctly.

System information (as much as possible)

Joomla 3.6.0

Template in the screenshot is based on protostar, but error was reproducible using all other templates that were installed (protostar and beez3)

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

For selects (type="list") to attach chosen JS (behaviour + styling)

  • it is a requirement that you fields of type list have
    class="advancedSelect"

For radios, do not use class="btn-group" !! instead

  • either use no CSS class
  • or some CSS class that is not relating to JS-based styling via the template

@Fedik
what do you think about:

Template styling can be supported if a new requirement for templates would be introduced:

  • every compliant template would need to register its JS-based styling, via a new JS method,

(notice the "sel" that allow applying the stylings to a specific container)

Joomla.applyTemplateStyles = function (sel)
{
    sel = typeof sel != 'undefined' ? sel : 'body'; 

    // Turn radios into btn-group
    jQuery(sel).find('.radio.btn-group label').addClass('btn');

    jQuery(sel).find('fieldset.btn-group').each(function() {
        if (jQuery(this).prop('disabled')) {
            jQuery(this).css('pointer-events', 'none').off('click');
            jQuery(this).find('.btn').addClass('disabled');
        }
    });
    ...
}

The subform "addRow()" method, would call the above method

  • passing the parent container div of the new set of fields

But now the above does not exist and we can not use it,

  • another option would be for the subform field to hard-code the bootstrap template stylings

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

The solution of hard-coding the JS-based styling is what i do myself now,

  • as i don't have a way to force current template to provide their JS-based styling

But currently great majority of templates

  • are just adding the default classes and register the same JS event handlers
  • and then only customizing the CSS rules for the default CSS classes

Thus best solution is to

  1. hard-code these JS-based stylings into the subform field,
  2. and check if method: Joomla.applyTemplateStyles exists, if it exists call it, otherwise call the built-in hard-coded stylings

the above is B/C friendly too

@Fedik
Copy link
Member

Fedik commented Jul 25, 2016

check if method: Joomla.applyTemplateStyles exists, if it exists call it, otherwise call the built-in hard-coded stylings

no, it is bad solution

Just need to accept that we need the Events API for interact between different part of fronted scripts, and stop hardcoding.
There already was couple suggestion #1260 and #6357 (here even fixed a current issue template.js#L10)

Also I seen some idea on JAB 2016

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

Hard coding is what templates are doing now, i suggested to stop doing it,

About events API , i would say yes , and since this is to be added,

  • i agree, it is best not to add something like: Joomla.applyTemplateStyles

Now about not hard-coding it inside the subform field, i will disagree

  • since there are similar purpose stuff, that are already hardcoded inside the subform
  • also it will be temporary without an new public method

thus it can be replaced when the other solutions are available without a B/C break ? !

@Fedik
Copy link
Member

Fedik commented Jul 25, 2016

since a lot of stuff is already hardcoded inside the subform

bad argumen, you know 😄

also it will be temporary without an new public method

as practice shows mostly every temporary thing stays forever 😉

The subform field has event 'subform-row-add' on each new row, so it can be used for style the stuff.

$(document).on('subform-row-add', function(event, row){
// here is the code to do some stuff with the field in `row`
});

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

since a lot of stuff is already hardcoded inside the subform

bad argument, you know 😄

yes

as practice shows mostly every temporary thing stays forever 😉

yes , and now that i think of it is not "worthy" enough of a temporary solution ?

  • bettermake a note for people that it is a requirement, not to use JS-based stylings (yet) 😉

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

Then what needs to be done for this issue

is to document requirements ??:

  • use class="advancedSelect" for attaching chosen JS
  • do not use CSS styling that requires JS like: (bootstrap) "btn-group"

@Fedik
Copy link
Member

Fedik commented Jul 25, 2016

yes, for make <select> use Chosen.js need to add class advancedSelect to the field:

<field name="languagesub" type="list" class="advancedSelect" ... 

for "btn-group": if there a way do not use JavaScript, it would be perfect, otherwise need to tweak a script in template.js to make it work. Some one can use a fix from a closed PR template.js#L10 as example (need just replace var $target = $(event.target) to var $target = $(row || document))

$(document).on('subform-row-add', function(event, row){
// here is the code to make "btn-group" work 
});

@ApolloLV
Copy link
Author

Well, the Documentation for Subforms is really sparse at the moment. The only usable description of the different types of formats of subforms was on stackoverflow. Maybe I didn't look at the right places, but right now, there is nothing about sub form fields in form fields, nor repeatable fields, even though the code for repeatable fields already mentions that they are deprecated and that sub form fields should be used instead. Even the release announcements for 3.6.0 praised subforms without pointing to any usage instructions!

Anyways, I'm going to try the fix with assigning the additional class to the list.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11299.

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

@Fedik

you original issue had usage description:
#7829

but it never made into documentation ? , i see user posted it to stackoverflow

About attaching bootstrap behaviour / styling and making possible to use:
class="btn-group btn-yes-no"
in your fields, this may work:

/* Attach boostrap styling / behaviour to the inner contents of the given row */
$(document).on('subform-row-add', function(event, row)
{
    // Turn radios into btn-group
    $(row).find('.radio.btn-group label').addClass('btn');

    // Prevent clicks on disabled fields
    $(row).find('fieldset.btn-group').each(function() {
        if ($(this).prop('disabled')) {
            $(this).css('pointer-events', 'none').off('click');
            $(this).find('.btn').addClass('disabled');
        }
    });

    // Add btn-* styling to checked fields according to their values
    $(row).find('.btn-group label:not(.active)').click(function()
    {
        var label = $(this);
        var input = $('#' + label.attr('for'));

        if (!input.prop('checked')) {
            label.closest('.btn-group').find('label').removeClass('active btn-success btn-danger btn-primary');
            if (input.val() == '') {
                label.addClass('active btn-primary');
            } else if (input.val() == 0) {
                label.addClass('active btn-danger');
            } else {
                label.addClass('active btn-success');
            }
            input.prop('checked', true);
            input.trigger('change');
        }
    });

    // Similar
    $(row).find('.btn-group input[checked="checked"]').each(function()
    {
        var input = $(this);
        if (input.val() == '') {
            input.parent().find('label[for="' + input.attr('id') + '"]').addClass('active btn-primary');
        } else if (input.val() == 0) {
            input.parent().find('label[for="' + input.attr('id') + '"]').addClass('active btn-danger');
        } else {
            input.parent().find('label[for="' + input.attr('id') + '"]').addClass('active btn-success');
        }
    });

    // Also add coloring for chosen ?
    // ... add if you need
}

@ApolloLV
Copy link
Author

Is this related to this other bug I encountered when using calendar fields?
subform-bug2
Should I open a seperate bug report for this? This weird warning is displayed upon page load and all additional calendar fields in subforms don't work. How did this feature make it into 3.6.0 after all?
Kind of disappointed.

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

How did this feature make it into 3.6.0 after all?

Ok now i have lost you,
so far we have not found any bugs for it in this thread

  • the stylings are not a bug, some better documentation will help avoid frustration
  • also it is undocumented which Joomla form fields types are have been tested to work with it

A mistake was that a new property was not added to JFormField Class ,
$supportSubForm = false; // defaults to false !

And then

  • every field that has been tested , would have this property to true,
  • and subform will only render fields that have it and replace the others with a notice asking to remove field from the subform

@ApolloLV
Copy link
Author

Oh, I just assumed that subforms were supposed to work with any JFormField class, my bad.

So I will have to go back to using text fields for repeatable calendar dates then?

@ggppdk
Copy link
Contributor

ggppdk commented Jul 25, 2016

Test which fields work,
and report not working here

  • since there was no documentation of the supported fields,
  • and neither the property (FLAG) to add warning for non-supported fields (yet)

it is natural that you assumed that all fields would work !

about calendar fields, i guess you should use text input for now

@dgrammatiko
Copy link
Contributor

@ApolloLV I am working on an update of the calendar field that will be compatible with sub forms: #11138

@Fedik
Copy link
Member

Fedik commented Jul 26, 2016

I just assumed that subforms were supposed to work with any JFormField class, my bad

the subform field work with all fields, it just not all fields works with subform field 😄

Mostly fields that have additional javascript logic are broken, because they did not made to live in the dynamic world. If you set multiple="false" you will see all works perfect. 😉

There already was issue about this, cannot find.

@ApolloLV
Copy link
Author

ApolloLV commented Jul 26, 2016

@Fedik This one is probably related, since it's also javascript logic (In this case styled radio buttons) being broken upon creating additional sub form fields: #10718
@dgt41 Looks awesome, I am going to try that one out as soon as it makes it into a release. Sadly, the project I am working on right now will not allow for any code changes in base.

@roland-d
Copy link
Contributor

roland-d commented Aug 3, 2016

@ggppdk You said "also it is undocumented which Joomla form fields types are have been tested to work with it" does someone know which ones are tested? If so, we should ask to update the documentation at https://docs.joomla.org/index.php?title=Subform_form_field_type

"the stylings are not a bug, some better documentation will help avoid frustration "
Could you update the documentation with what you know? Thanks.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11299.

@gaelicwinter
Copy link

I can confirm the sample code given by @ggppdk works works when added to the layout file although I had to use jQuery instead of $, presumably because of Bootstrap.

@Sophist-UK
Copy link
Contributor

The cause of radio button groups not being styled is because groups in the initial page load are styled by code in template.js, and this code is not run when new button groups are created by subform-repeatable.js. The same is true of Chosen dropdown styling.

I will fix this - by moving the code into a global function which is called from template.js but can also be called from subform-repeatable.js. If anyone disagrees with this approach please comment now to avoid me wasting effort on a PR which will not be merged.

@ggppdk
Copy link
Contributor

ggppdk commented Feb 15, 2018

@Sophist-UK

exactly what i said in this issue and in a different issue a year or more ago
and at the time i was told it is not a good solution, and a better solution should be implemented
i do not remember details about it

maybe now nobody will complain about it and your PR will be merged

Quoting my comment

every compliant template would need to register its JS-based styling, via a new JS method

thus template specific styling can be applied

i think the negative comment about the above, is that a complete interface or something should be defined that templates will use, i don't remember exactly

@Sophist-UK
Copy link
Contributor

I have done this in a way that we do not need a new standard JS method. Although I have used a standard method in isis, protostar and beez, it doesn't need to be. Instead I use the subform.row.add jQuery event to style the new subform row.

@ggppdk
Copy link
Contributor

ggppdk commented Feb 18, 2018

You have done exactly what i was talking above,

  1. add a new JS method to contain the template styling
  2. call on page load for window contents
  3. call it on when a new row is added on the container

so there is nothing different about it
exactly what it was denied a year ago when i had asked for exactly this

instead of calling it
Joomla.applyTemplateStyles

you called it
Joomla.TemplateJs

@Sophist-UK
Copy link
Contributor

Nope - using events was what other people were suggesting whilst you were suggesting that the new method was called directly from subform add by having a standardised method name.

@ggppdk
Copy link
Contributor

ggppdk commented Feb 18, 2018

yes it is as you say
that is the only difference that you did,

and it is really not a good idea
please see why in my answer in your PR

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/11299

@ghost
Copy link

ghost commented Feb 19, 2018

closed as having Pull Request #19722

@Sophist-UK
Copy link
Contributor

See comments in #19722 and #20221 / #20222 / #20224 / #20225.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants