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

WIP - Upgrade Select2 to version 4 #20825

Closed
wants to merge 2 commits into from
Closed

Conversation

colemanw
Copy link
Member

Overview

Upgrades the Select2 library used for rendering autocompletes, searchable selects, tagging inputs, etc.

Before

Select2 v3.5

After

Select2 v4.0.13

Technical Details

The 4.0.x series contains a compatibility layer which will help during this transition. That's why this PR doesn't take us all the way to the latest 4.1.x.

@civibot
Copy link

civibot bot commented Jul 11, 2021

(Standard links)

@civibot civibot bot added the master label Jul 11, 2021
@colemanw
Copy link
Member Author

@monishdeb I've rebased the work you did in #11969 to give it another try and see how far we can get. So far, clicking around I see multiple problems and console errors, so there's more work to do. I'll keep this PR as a draft for now; feel free to jump in with additional commits (you can create a PR against this PR by checking out my branch and pushing it to your fork).

@JoeMurray
Copy link
Contributor

@monishdeb could you work on this with @colemanw over the next little while to try to complete? There are some compatibility layers that he has found that should help.

@monishdeb
Copy link
Member

Thanks @colemanw for putting this PR. Yes, I'll make the additional changes on my branch and I am hoping you can help me with some compatibility issues which I was unable to fix earlier :(

@mlutfy
Copy link
Member

mlutfy commented Jul 13, 2021

(afform tangent)

I tested on top of 5.40 and things did not dramatically explode, which is cool (it's a big upgrade and presumably a lot of work went into this).

I had run into a bug with afform on WordPress, where select2 is broken if we are using a recent block theme, such as TwentyTwentyOne (older themes work fine). I will try to open a bug elsewhere, but since Eileen pointed me to this PR, I just wanted to say that it does seem to help on WP.
Without the patch, the form has JS errors that cause weird rendering. With the patch, the select2 renders, but has an empty list:

afform-2021-07-12_21-28

Edit: opened a separate bug for afform: https://lab.civicrm.org/dev/core/-/issues/2688

@monishdeb
Copy link
Member

Thanks, @mlutfy for reporting the issue, will try&fix the bug while working on the compatibility issues. I am pretty sure this is one of those issues which I haven't addressed yet but yeah, I'll let you know when it's fixed.

@colemanw
Copy link
Member Author

colemanw commented Jul 15, 2021

I've been trying to get EntityRef fields working, with discouraging results. Based on my testing, the 4.0.x series of Select2 maintains limited support for <input> type elements (which are the basis of current EntityRef fields), which evidently does not extend to supporting ajax.

I've tested several different versions of Select2 (4.0.0, 4.0.5, 4.0.10) and all give the same results - the "ajax" setting works when calling .select2() on a <select> element but not an <input>.

This would be a deal-breaker if we can't find a way to get that working. Yes, it would be good to transition our EntityRef field markup to use <select> instead of <input>, but it would need to be gradual. If we drop this in now it would require massive refactoring of core forms and BC breaks with any extensions using EntityRef fields.

Just to be sure I'm not missing something, @monishdeb did you ever get EntityRef fields working? @mlutfy when you say "things did not dramatically explode" did you test any EntityRef fields?

@monishdeb
Copy link
Member

monishdeb commented Jul 16, 2021

@colemanw sorry for the delayed response. No, I wasn't able to get the entityRef part resolved (using <input>). But earlier I made it partially work (at least with search) by using <select> element - commit and fixed it partially as per the screencast here #11969 (comment) but there were some missing pieces like the ability to create contact and Refine search were broken.

@colemanw
Copy link
Member Author

I wrote directly to the author of select2, @kevin-brown and he was kind enough to reply and confirm that <input> elements are indeed broken in the 4.0.x series, with no plans to fix them as they are completely removed from 4.1.x.

I see 2 paths forward:

  1. Put our own dev time into getting <input> elements working with the upstream select2 library. This would require myself or another Civi developer to grok the select2 codebase to see how it can be patched up. According to @kevin-brown:

    You would need to take a lot of inspiration from the base <select> adapter in order to make it work, but it's definitely achievable. Unfortunately it's going to take a decent amount of work to get it compatible with your existing use cases and you will most likely need to modify the Select2 source where the default data adapters are built in order to make it work.

  2. Patch our crmSelect2 wrapper to automatically replace an <input> element with a <select> during initialization. That's easy to do, and neatly solves the problem of supporting inputs. However it doesn't change the fact that multiselects will act differently, sending array data back to the server instead of a comma-separated string. A large audit will have to take place to ensure the new array format is being processed correctly on all forms that use multiselects.

Personally, I lean toward option 2 because both options require a lot of work, but the second option retires technical debt while the 1st just pays interest on the debt.

@alifrumin
Copy link
Contributor

We at AGH are interested in moving this forward and can contribute time reviewing/testing when the time comes.

@demeritcowboy
Copy link
Contributor

This has conflicts and has been unworked on for several months. Going to close but feel free to reopen.

@JoeMurray
Copy link
Contributor

@monishdeb this might be a good week to work on this and resubmit.

@JoeMurray
Copy link
Contributor

@colemanw 's option 2 seems like it is still a viable approach. One benefit of select2 being minimally maintained is that there are relatively few changes we would need to worry about happening on something we depend on.

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