Added option to load and enable select2 in ticket registration form #212

vedanshujain · 2018-05-28T17:31:46Z

We can remove this once select2 is added to the core. By default it will not be loaded or enabled, an option to enable it is also added.

iandunn

Overall this looks really good, great job!

[ Doh, accidentally submitted half-way through review, I'll edit to make my notes more descriptive ]

iandunn · 2018-05-28T20:09:21Z

camptix.php

 		wp_enqueue_style( 'camptix' );
+
+
+		if ( (bool) $this->options['select2_enabled'] ) {


I don't think we need an option for this; adding it doesn't break backwards-compatibiliity, and I don't imagine many users wanting to disable it. Here's some more info on how we typically approach options:

https://nacin.com/2011/12/18/in-open-source-learn-to-decide/
https://wordpress.org/about/philosophy/

I want a configuration because of accessibility (select2/select2#3744) . Looks like this is also the blocker for merging select2 in core. What do you think ? At the very least, we should enable by default.

That's a good point. They could always call wp_dequeue_script() from a plugin to disable it, but I realize that'd require some development experience.

It sounds like one our coworkers has already forked Select2 to make it accessible, so maybe we should switch to that instead?

If you feel like it, it might even be good to send a PR to selectWoo to merge in the upstream changes. That'd help us by fixing any bugs that haven't made it downstream; help selectWoo users for the same reason; and eventually help WP Core and Drupal, by making it easy to merge the a11y fixes upstream.

In general we try to be good open source citizens, but of course it always has to be balanced against time spent on our own priorities. This seems like a really good opportunity, though.

Or, rather than trying to merge downstream, maybe it'd be better to just cherry-pick commits from downstream and send PRs upstream? That may be more efficient, since any changes will need to be tested in either direction. Whichever you think is best :)

I have send in some PRs in wooSelect (woocommerce/selectWoo#18 and woocommerce/selectWoo#17) but it will take a while before they get merged. I want to wait till they get before we use wooSelect, since it doesn't have downstream changes, but we can use it right away as well. What do you think ?

It sounds like they'll be looking at it soon, so waiting is probably fine in this case. If it takes them longer than expected, then we can always go ahead and use the setup the latest master build now, and then update it after your PRs are merged.

iandunn · 2018-05-28T20:09:31Z

camptix.js


+    /**
+	 * Use select2 for dropdown
+      */


It looks like the indentation is off a bit in a few places around this line.

iandunn · 2018-05-28T20:10:35Z

camptix.js

+    /**
+	 * Use select2 for dropdown
+      */
+    $( document ).ready( function loadSelect2Country() {


The coding standards call for tabs instead of spaces.

We've got a custom phpcs.xml.dist file in Meta repo that describes the standards for WordCamp.org. Using phpcs makes it really easy to find any violations before committing. There's also a .githooks/pre-commit script in the Meta repo that you can use to automatically do PHP syntax checks, run phpcs, and run phpmd before committing.

I'd recommend setting those up, even if you only run them manually. It's a lot easier than memorizing everything at first, and more consistent.

iandunn · 2018-05-28T20:14:22Z

external/select2/js/select2.js

@@ -0,0 +1,5746 @@
+/*!


What do you think about using npm / package.json to register this as a dependency and include it, instead of manually bundling it?

That'd make it easier to get security notifications via npm audit and GitHub.

We'd still need to explicitly bundle it in the SVN repo, but that's no problem.

Agree that we should use npm. Lets wait for wooSelect, we can this push this as a separate change.

iandunn · 2018-05-28T20:28:14Z

camptix.php

-            <label class="tix-yes-no description"><input type="radio" name="<?php echo esc_attr( $args['name'] ); ?>" value="1" <?php checked( $args['value'], true ); ?>> <?php _e( 'Yes', 'camptix' ); ?></label>
-            <label class="tix-yes-no description"><input type="radio" name="<?php echo esc_attr( $args['name'] ); ?>" value="0" <?php checked( $args['value'], false ); ?>> <?php _e( 'No', 'camptix' ); ?></label>
-        </div>
+	function field_enable_select2( $args ) {?>


Kudos for doing coding standards fixes, and for doing them in a separate commit :)

The Development Practices doc has some helpful points on writing commit messages.

Use the imperative mood when possible: “Relax term ID comparisons…” instead of “Relaxes term ID comparisons…” [ from Core handbook ]

Something like Replace spaces with tabs., or just a generic Apply coding standards. would fit a little bit better.

It's probably best to avoid updating the commit messages, rebasing, and force-pushing until we've finished reviewing everything else, though, since the force-push will wipe out a lot of the GitHub comments.

iandunn · 2018-05-28T20:29:38Z

camptix.php

 						?>
 						<input type="hidden" name="tix_attendee_info[<?php echo esc_attr( $i ); ?>][ticket_id]" value="<?php echo intval( $ticket->ID ); ?>" />
-						<table class="tix_tickets_table tix-attendee-form">
+						<table class="tix_tickets_table tix-attendee-form <?php echo ( isset($this->options['select2_enabled']) && (bool) ( $this->options['select2_enabled'] ) ) ?


This is one of the places we'd like to add it. It'd also be use it for the currency dropdown and the Summarize fields in wp-admin.

There is a separate patch for currency, since its not a part of camptix. Or am I confusing with a different currency dropdown ?

Yeah. CampTix has a currency dropdown on the Tickets > Setup > General screen. The Summarize one is under Tickets > Tools.

Ah ok. Added.

iandunn · 2018-05-28T20:42:56Z

I tested it locally and it worked well for me. I'll take a closer look at the code tomorrow, but those should give you a few things to look at.

We can remove this once select2 is added to the core. By default it will not be loaded or enabled, an option to enable it is also added.

coreymckrill

To clarify, the idea here is to add the Select2 lib in for now, and then switch it out for selectWoo once your PRs are merged?

If so, we should talk about that a bit more, it seems rather drastic to me...

I added a couple of minor inline comments, but otherwise this looks good 👍

coreymckrill · 2018-06-08T23:19:24Z

camptix.php

+
+
+		if ( (bool) $this->options['select2_enabled'] ) {
+			wp_register_style( 'select2', plugins_url( '/external/select2/css/select2.min.css', __FILE__ ), array(), $this->version );


The version numbers used to register the stylesheet and script should be the version of Select2, not CampTix.

coreymckrill · 2018-06-08T23:27:35Z

camptix.php

+			wp_register_style( 'select2', plugins_url( '/external/select2/css/select2.min.css', __FILE__ ), array(), $this->version );
+			wp_register_script( 'select2', plugins_url( '/external/select2/js/select2.min.js', __FILE__ ), array(), $this->version );
+
+			wp_enqueue_style( 'select2' );


Just noting here at that since the Select2 assets will always be both registered and enqueued if the option is enabled, you can save a step by just calling the enqueue functions with all the same parameters, and they will take care of the registration as well.

But I guess another question is, should the assets always be enqueued, or only for certain views?

vedanshujain requested review from coreymckrill and iandunn May 28, 2018 17:31

iandunn reviewed May 28, 2018

View reviewed changes

vedanshujain added 2 commits June 7, 2018 19:29

Added option to load and enable select2 in ticket registration form

ca31be7

We can remove this once select2 is added to the core. By default it will not be loaded or enabled, an option to enable it is also added.

Cosmetic changes + also added select2 for "Summarize by" field.

e9542b0

vedanshujain force-pushed the select2-registration-form branch from 0b4787b to e9542b0 Compare June 7, 2018 14:00

coreymckrill reviewed Jun 8, 2018

View reviewed changes

		wp_enqueue_style( 'camptix' );


		if ( (bool) $this->options['select2_enabled'] ) {



		if ( (bool) $this->options['select2_enabled'] ) {
		wp_register_style( 'select2', plugins_url( '/external/select2/css/select2.min.css', __FILE__ ), array(), $this->version );

Added option to load and enable select2 in ticket registration form #212

Are you sure you want to change the base?

Added option to load and enable select2 in ticket registration form #212

Uh oh!

Conversation

vedanshujain commented May 28, 2018

Uh oh!

iandunn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iandunn May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iandunn May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iandunn May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iandunn May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iandunn May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iandunn May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iandunn commented May 28, 2018

Uh oh!

coreymckrill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iandunn left a comment •

edited

Loading

iandunn May 28, 2018 •

edited

Loading

iandunn May 28, 2018 •

edited

Loading

iandunn May 28, 2018 •

edited

Loading

iandunn May 28, 2018 •

edited

Loading

iandunn May 28, 2018 •

edited

Loading

iandunn May 28, 2018 •

edited

Loading