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

Move from chosen library to select2 and remove deprecated asset enqueuing strategy #1645

Merged
merged 12 commits into from Dec 13, 2018

Conversation

Projects
None yet
5 participants
@jom
Copy link
Member

jom commented Dec 11, 2018

Fixes #1202
Fixes #1358

See the issue for the reasons we're moving away from chosen.

Changes proposed in this Pull Request:

  • Removes support for old asset enqueuing strategy deprecated in #1354.
  • Deprecate the old chosen library.
  • Loads select2 asset for category select and multiselect.

This will eventually also be used in the backend for background pulled select fields (#846).

Testing instructions:

  • This will involve quite a bit of testing for compat with popular plugins and themes.
  • Generally, make sure all frontend screens (all shortcodes) have the same formatting as master and the <select /> fields get the fancy select2 treatment.

Proposed changelog entry for your changes:

  • Developer: Moved from chosen JS library to select2.

@jom jom force-pushed the change/move-to-select2 branch from c680420 to fcb3007 Dec 11, 2018

@jom jom added this to the 1.32.0 milestone Dec 11, 2018

@spencerfinnell

This comment has been minimized.

Copy link
Collaborator

spencerfinnell commented Dec 12, 2018

I think the ‘enable’ filters should stay. Use that value as the base value for the new ones maybe?

@jom

This comment has been minimized.

Copy link
Member Author

jom commented Dec 12, 2018

@spencerfinnell Enqueue chosen with the enable filter (job_manager_chosen_enabled)? I was considering that and don't mind it if it will prevent issues (especially with older versions) of your themes.

I suppose job_manager_chosen_enabled has also been an alias for "use WPJM core's multiselect scripts on this page" as well. We could set it as the default for now and maybe add a deprecation notice.

@jom jom force-pushed the change/move-to-select2 branch from 78511d0 to 329dbfc Dec 12, 2018

Deprecate `JOB_MANAGER_TEST_NEW_ASSET_BEHAVIOR` constant
Themes and plugins now need to explicitly tell WPJM when the frontend style is needed.

@jom jom changed the title [WIP] Move from chosen library to select2 for selects [WIP] Move from chosen library to select2 and remove deprecated asset enqueuing strategy Dec 12, 2018

Register (but don't enqueue) select2 for admin usage
Used in our plugins and soon in the job listing editor in WP admin

@jom jom changed the title [WIP] Move from chosen library to select2 and remove deprecated asset enqueuing strategy Move from chosen library to select2 and remove deprecated asset enqueuing strategy Dec 12, 2018

@jom jom requested review from alexsanford and donnapep Dec 12, 2018

Trigger event when application details is entirely visible
This helps with select2 issue in plugins.
@tripflex

This comment has been minimized.

Copy link
Collaborator

tripflex commented Dec 12, 2018

Please keep in in the loop regarding this, the switch from Chosen to Select2 will affect a lot of my plugin's feature (working on an update for this) including conditional logic and other things that are specifically tied to Chosen.JS

@jom

This comment has been minimized.

Copy link
Member Author

jom commented Dec 12, 2018

@tripflex This is slated for our mid-January release. We should have a beta in the next week, though.

@spencerfinnell I changed it so job_manager_select2_enabled defaults to true if job_manager_chosen_enabled filters to true (default false).

@spencerfinnell

This comment has been minimized.

Copy link
Collaborator

spencerfinnell commented Dec 12, 2018

Thanks @jom -- is there a timeline for this release? It's very impactful as @tripflex mentioned and a roadmap/ETA will be helpful.

@jom

This comment has been minimized.

Copy link
Member Author

jom commented Dec 12, 2018

@spencerfinnell It is on the milestone https://github.com/Automattic/WP-Job-Manager/milestone/18

It's very impactful

With continuing to package chosen and the legacy filter, where are you primarily seeing the impact with the PR as-is?

@alexsanford
Copy link
Contributor

alexsanford left a comment

This looks good to me. Just one brief question below.

I did some quick testing on the shortcode pages and things seemed to work fine with select2. We'll do more comprehensive testing in beta, so I don't have any blockers 🙂

@@ -277,6 +287,8 @@ module.exports = function( grunt ) {
grunt.loadNpmTasks( 'grunt-wp-readme-to-markdown');
grunt.loadNpmTasks( 'grunt-zip' );

grunt.registerTask( 'update-assets', [ 'copy:select2' ] );

This comment has been minimized.

@alexsanford

alexsanford Dec 12, 2018

Contributor

Does this task need to be run manually? Should we document it somewhere?

This comment has been minimized.

@jom

jom Dec 12, 2018

Author Member

It does need to be called manually. It will probably happen when we are prepping the release PR (not packaging/deploying it as it will be committed). I'll add it to our docs when we merge.

@spencerfinnell

This comment has been minimized.

Copy link
Collaborator

spencerfinnell commented Dec 12, 2018

@jom thanks. Just styles mainly. But the milestone timeline works for me. We can get a compatibility release out by then.

@donnapep

This comment has been minimized.

Copy link
Contributor

donnapep commented Dec 12, 2018

I changed it so job_manager_select2_enabled defaults to true if job_manager_chosen_enabled filters to true (default false).

Drive-by comment, but am thinking it would be better to name the filter something that is not tied to the JS library name for better future-proofing.

@jom

This comment has been minimized.

Copy link
Member Author

jom commented Dec 12, 2018

@donnapep I considered that, but (our plugins included) will break if we change the library because we call it in our non-core plugin scripts.

@spencerfinnell

This comment has been minimized.

Copy link
Collaborator

spencerfinnell commented Dec 12, 2018

Drive-by comment, but am thinking it would be better to name the filter something that is not tied to the JS library name for better future-proofing.

Which is why theres a lot of side-stepping now. I'd say do what is needed for backwards compat but now but moving forward call it just something like _enhanced_select_enabled

@jom

This comment has been minimized.

Copy link
Member Author

jom commented Dec 12, 2018

@spencerfinnell @donnapep I imagined it as a promise that we'll be including select2 for WPJM related themes/plugins to use (without an abstraction layer in place) so they didn't need to package it as well. We would then be able to trigger a deprecation notice (like we are for chosen) if we ever needed to get rid of it. For now, I've added d7ba2ba.

I've updated the PRs for Resumes and Applications. In those implementations, until we package select2 with them, I've tried to add checks for the existence of $.fn.select2 before using it.

@jom jom merged commit dc41942 into master Dec 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jom jom deleted the change/move-to-select2 branch Feb 18, 2019

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