`crud.field()` js library bugs

[tabacitu]

@pxpm said (Originally posted by @pxpm in https://github.com/Laravel-Backpack/CRUD/pull/4312#pullrequestreview-944434626)

---

Hey @tabacitu amazing work here! So I am just giving this a round up review, I am not going to work on fixes unless you tell me to.

So until now I figured out that: 1) It does not work with arrayed name fields like date_range. It will throw an error in wrapper because of the bp-field-name

Yeah, we SHOULD fix this.

2) jquery date_picker and datetime_picker catch the change event when initializing, but then don't catch the change when you select a date from the picker. (also happens with: address_algolia fields)

Yeah, we MUST fix this.

3) In the date html field, if you are manually typing the date, the change event is triggered in every keystoke when you start inputing the year. Same happens with datetime, time, month, week

I'm ok with this... I don't think we should do anything about this, it's just the way these work. Wouldn't you agree? I'd say WON'T DO.

crud.field('date').onChange(function(e, value) {
    console.log('date')
    var now = new Date();
    var today = new Date(now.getFullYear(), now.getMonth(), now.getDate());
    var selectedDate = Date.parse(value);
    if (selectedDate < today) {
        new Noty({
            type: "error",
            text: "date cant be lower than today"
          }).show();
    }
});

4) for hasOne with dot notation the bp-field-name should probably be address.street so developer can target it with address.street, otherwise it needs to know beforehand that we will change the name to address[street]. At the moment trying to add address.street as a field name in javascript will break the jquery search, changing to passing full strings to the search query makes it work:

Thanks, we COULD fix this.

constructor(fieldName) {
            this.name = fieldName;
            this.wrapper = $('[bp-field-name="'+ this.name +'"]');
            this.input = $('[bp-field-main-input][name="'+ this.name +'"]');
            // if no bp-field-main-input has been declared in the field itself,
            // assume it's the first input in that wrapper, whatever it is
            if (this.input.length == 0) {
                this.input = $('[bp-field-name="'+ this.name +'"] input, [bp-field-name="'+ this.name +'"] textarea, [bp-field-name="'+ this.name +'"] select').first();
            }
            this.value = this.input.val();
        }

5) The multiple selects use an hidden input with empty value, not to store the field itself but to submit when nothing is selected. I could not target the selects with name or name[]. I tried to add:

'wrapper' => [
   'class' => 'form-group col-md-6',
   'bp-field-main-input' => 'name[]'
],

No avail here.

Great catch! We MUST fix this.

6) can't make any js editor work with onChange, neither by typing or moving focus out.

Another great catch, we MUST fix this.

7) video field does not catch the event too.

SHOULD DO.

8) color_picker jquery plugin will trigger the change event when you drag to chose the color triggering hundreds of times in a short period.

🤷‍♂️ I'm ok with it... to be honest... I'd say WON'T DO.

9) icon_picker does not work too

SHOULD DO

10) I haven't tried this with repeatable, but we should consider the "events/hooks" aproach for table as this script has no effect on table field.

COULD DO

[tabacitu]

@pxpm heads-up, I've moved the bugs you found here. To fix that branch let's use:

• this thread to keep track of bugs and testing
• this demo branch to easily test the features
• in terms of bug fixing, we can start with the ones you found (above);
• one PR for each bugfix, pointing to crud-form-javascript-library;

[tabacitu]

@pxpm let's centralize our testing in this table, using this demo branch. Legend:

• ? haven't tested
• ✅ tested, works
• 🟨 tested, doesn't work and it should
• ❌ doesn't work but we're not going to do it right now
• ⛔️ not applicable

Field	onChange()	hide()	show()	enable()	disable()	require()	unrequire()	Custom
checkbox	✅	✅	✅	✅	✅	✅	✅	✅ check(), ✅ uncheck()
checklist	✅	✅	✅	✅	✅	✅	✅	-
checklist_dependency	✅	✅	✅	✅	✅	✅	✅	-
color	✅	✅	✅	✅	✅	✅	✅	-
custom_html	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️
date	✅	✅	✅	✅	✅	✅	✅	-
datetime	✅	✅	✅	✅	✅	✅	✅	-
email	✅	✅	✅	✅	✅	✅	✅	-
enum	✅	✅	✅	✅	✅	✅	✅	-
hidden	✅	✅	✅	✅	✅	✅	✅	-
month	✅	✅	✅	✅	✅	✅	✅	-
number	✅	✅	✅	✅	✅	✅	✅	-
password	✅	✅	✅	✅	✅	✅	✅	-
radio	✅	✅	✅	✅	✅	✅	✅	-
range	✅	✅	✅	✅	✅	✅	✅	-
select (1-n relationship)	✅	✅	✅	✅	✅	✅	✅	-
select_grouped	✅	✅	✅	✅	✅	✅	✅	-
select_multiple (n-n relationship)	✅	✅	✅	✅	✅	✅	✅	-
select_from_array	✅	✅	✅	✅	✅	✅	✅	-
summernote	✅	✅	✅	✅	✅	✅	✅	-
text	✅	✅	✅	✅	✅	✅	✅	-
textarea	✅	✅	✅	✅	✅	✅	✅	-
time	✅	✅	✅	✅	✅	✅	✅	-
upload	✅	✅	✅	✅	✅	✅	✅	-
upload_multiple	✅	✅	✅	✅	✅	✅	✅	-
url	✅	✅	✅	✅	✅	✅	✅	-
view	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️
week	✅	✅	✅	✅	✅	✅	✅	-
address_algolia PRO	✅	✅	✅	✅	✅	✅	✅	-
address_google PRO	✅	✅	✅	✅	✅	✅	✅	-
browse PRO	✅	✅	✅	✅	✅	✅	✅	-
browse_multiple PRO	✅	✅	✅	✅	✅	✅	✅	-
base64_image PRO	✅	✅	✅	✅	✅	✅	✅	-
ckeditor PRO	✅	✅	✅	✅	✅	✅	✅	-
color_picker PRO	✅	✅	✅	✅	✅	✅	✅	-
date_range PRO	✅	✅	✅	✅	✅	✅	✅	-
date_picker PRO	✅	✅	✅	✅	✅	✅	✅	-
datetime_picker PRO	✅	✅	✅	✅	✅	✅	✅	-
easymde PRO	✅	✅	✅	✅	✅	✅	✅	-
icon_picker PRO	✅	✅	✅	✅	✅	✅	✅	-
image PRO	✅	✅	✅	✅	✅	✅	✅	-
relationship PRO	✅	✅	✅	✅	✅	✅	✅	-
repeatable PRO	✅	✅	✅	✅	✅	✅	✅	-
select2 (1-n relationship) PRO	✅	✅	✅	✅	✅	✅	✅	-
select2_multiple (n-n relationship) PRO	✅	✅	✅	✅	✅	✅	✅	-
select2_nested PRO	✅	✅	✅	✅	✅	✅	✅	-
select2_grouped PRO	✅	✅	✅	✅	✅	✅	✅	-
select_and_order PRO	✅	✅	✅	✅	✅	✅	✅	-
select2_from_array PRO	✅	✅	✅	✅	✅	✅	✅	-
select2_from_ajax PRO	✅	✅	✅	✅	✅	✅	✅	-
select2_from_ajax_multiple PRO	✅	✅	✅	✅	✅	✅	✅	-
table PRO	✅	✅	✅	✅	✅	✅	✅	-
tinymce PRO	✅	✅	✅	✅	✅	✅	✅	-
video PRO	✅	✅	✅	✅	✅	✅	✅	-
wysiwyg PRO	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️	⛔️

[pxpm]

@tabacitu I've added some PR's that cover alot of scenarios but I didn't update the table, I think it's better that when you test the PR's and they work you do the table update as a "I confirmed it works".

Cheers

[tabacitu]

@tabacitu I've added some PR's that cover alot of scenarios but I didn't update the table, I think it's better that when you test the PR's and they work you do the table update as a "I confirmed it works".

Exactly. That's what I've been doing 😉 Great minds think alike 😅🙏

[tabacitu]

TODO:

• [x] change JS event names according to https://github.com/Laravel-Backpack/CRUD/pull/4335 - and the fields that use them accordingly;
• [x] @promatik - make all other fields trigger backpack:field.change on keyup instead of focusout - done in https://github.com/Laravel-Backpack/CRUD/pull/4400
• [x] @promatik - the change() parameters would probably be better as field, event instead of event, value, name, type; just easier to work with field.name, field.value etc - see PR https://github.com/Laravel-Backpack/CRUD/pull/4400
• [x] @promatik - why can we do enable(false) but we can't do disable(false)? that's what I tried - done in https://github.com/Laravel-Backpack/CRUD/pull/4425
• [x] @pxpm in repeatable let's not re-do the selectors we have in CrudField; instead, let's use the CrudField API;
• [x] @tabacitu - avoid triggering the change() event on initial load (form_fields_script.blade.php line 31); done in https://github.com/Laravel-Backpack/CRUD/pull/4427
• [x] @pxpm - inside the constructor - do we really need to look up the tree, at ancestors?
• [x] @pxpm - CrudField selector uses *=, that needs to be replaced because if you have two fields with similar names (like name and name_and_surname we'll get into trouble; addressed in https://github.com/Laravel-Backpack/CRUD/pull/4435
• [x] @tabacitu - should we add aliases for enableWhen(), disableWhen() etc.? Would that make the API more readable or intuitive? NO
• [x] @tabacitu - triple-check how to use date_range, and if it's intuitive
• [x] @tabacitu - triple-check how to use checklist, and if it's intuitive
• [ ] @tabacitu - triple-check how to use checklist_dependency, and if it's intuitive
• [x] @tabacitu - write docs for it - https://github.com/Laravel-Backpack/docs/pull/349
• [x] include example in the demo for it (easy for us to test too)
• [x] @pxpm - change event names so that they work both with jQuery and JS, because right now the dot is treated differently (jQuery reads backpack:field.show as backpack:field with show as argument, JS reads it as backpack:field.show); maybe we use:
  • backpackField:show
  • bpField:show (ugly)
  • field:show (prettiest, but could maybe conflict with other stuff?)
  • CrudField:show - 🏆
• [ ] @promatik - bye bye jQuery? 👀 https://github.com/Laravel-Backpack/CRUD/pull/4391

[pxpm]

@tabacitu from my last tests (with pending pr's merged) it's only missing:

• repeatable
• enum
• address_google
• checklist_dependency
• date_range

[tabacitu]

IMPORTANT - single-source-of-truth branches we're using:

• CRUD - https://github.com/Laravel-Backpack/CRUD/pull/4312
• PRO - https://github.com/DigitallyHappy/backpack-pro/pull/22
• DEMO - https://github.com/Laravel-Backpack/demo/pull/384
• DOCS - https://github.com/Laravel-Backpack/docs/pull/349

From now on

• all PRs have to be surgical, and pointing to one of those branches;
• we point out issues in those PRs with inline comments, create a PR to fix it; then when the PR gets merged into it we resolve the issues;
• one by one, let's find bugs, fix them, and finish all of the above;

Let's do this!!! 💪🎉

[tabacitu]

I'm SUPER-happy to announce that I've just tested this the 3rd time and I'm happy with it, I think it's FINAL:

• all fields work as expected
• syntax is clear for all fields
• docs are ready
• demo is ready