Implement react-select for our select input components #515

nerrad · 2018-06-08T19:44:34Z

Problem this Pull Request solves

Right now in a few different places we are using chosen for a fancy select to help with auto-complete ux etc (Attendee Mover add-on, Barcode Scanner). Although chosen is okayish, there are some challenges bringing it into react for usage (encountered while working on the Barcode Scanner). Along with that, chosen has a number of accessibility issues to address. In some research, I came across react-select and although version2 is in beta it introduces some great flexibility and while still not fully accessible (nothing is really yet), it is somewhat accessible and they are working on improving that. Implementing this as a component for our select needs allows us flexibility in the various uses we may have for selects (single select, multi-select etc). Brent and I chatted about this in slack and he was in agreement with this as well.

Along with the implementation of react-select, there's a number of other under the hood changes in this pull request.

Abstracting out things specific to model related data interaction into the `data/model` folder.

Our existing selects had a lot of boilerplate code for building query's etc that will likely get used elsewhere beyond just selects. It is also more fitting to have it connected with the data business logic for our modules. So for the new selects, this has been abstracted more so that:

file	purpose
data/model/base.js	This is a base function containing the logic for generating queryString from passed in arguments.
data/model/model-names.js	this simply returns an array of modelNames from the data exposed in the view via eejs.
model/event/index.js	It is expected that each model will have its own folder in this path containing functionality specific to the model. In this particular file are a number of functions related to prepping things for a event REST query. I fully expect this will be extracted even further into specific files eventually.

Changing folder structure to more clearly reflect the component context.

Instead of components/selection, I've added the new selects in components/form/select. This could likely even be further refined to be components/form/select/modelName for the individual models when necessary.

Building a base `ModelSelect` component with an accompanying `withSelect` HOC wrapping it.

The ModelSelect component provides all the logic that exposes the react-select component for use. Usage allows for indicating what model and modelentities are to be used for building the select as well as passing through the react-select component options to configure that component itself (via a selectConfiguration prop).

This then allows add-ons to utilize this as a base for their own composite components using this.

There's an example of a new EventSelect component that is composed of the ModelSelect. It becomes much easier to construct any selects that utilize data from a model endpoint.

What remains to do?

Do some documentation of how to utilize ModelSelect. I want to hold off doing this until its had a review (and merged into master).
It's possible once we start implementing this we may want to play with the default styling of the select output a bit. However, that's something we can do when we come to it. react-select has a specific way it can be styled programatically so IF we decide to change the styles we'll want to investigate that.
I think I may need to tweak how I'm using derivedStateFromProps in ModelSelect yet. Basically I should only be changing state if there is a change detected for the incoming props with the current state (see example here). I can probably utilize memoize a bit in the some of the helper functions as well. This can be something done after this pull is merged and we are using it in blocks (so we can measure improvements).

Important things to note for converting over older selects to this new component:

For the most part, the new EventSelect component can be used as a model (pun not intended) for how the conversion works. However it's important to note that the signature for the event callbacks are specific to react-select so you need to keep that in mind when handling the event in the top-level component (i.e. EventAttendees). Namely:

`OnChange` callback

This differs from the WP <SelectControl> in that instead of the signature recieving back the id that was selected, there are two arguments passed into callback with the following schema:

Argument	Possible types	Details
selection	null \| undefined \|Array <Object> \| Object	The object is in the shape of `{ value: 10, label: 'Some label' }`
action	Object	An object with the shape of `{ action: <string> }` and the values are `select-option`, `deselect-option`, `remove-value`, `pop-value`, `set-value`, `clear`, `create-option`

How has this been tested

I've added new jest and enzyme tests (and modified any existing tests broken in the process) and ensured they passed.
I've tried this out on a branch based off of the EventAttendee block branch to ensure it works as expected.

Checklist

I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs
User input is adequately validated and sanitized
all publicly displayed strings are internationalized (usually using esc_html__(), see https://codex.wordpress.org/I18n_for_WordPress_Developers)
My code is tested.
My code follows the Event Espresso code style.
My code has proper inline documentation.
My code accounts for when the site is Maintenance Mode (MM2 especially disallows usage of models)

This is populated via eejs.data from the server.

This uses the react-select component

…r react-select component

tn3rb

Overall everything looks exemplary! Good job!

I do have the following list of items:

a few suggestions for minor improvements
a couple of questions
one potentially serious reservation

tn3rb · 2018-06-11T16:12:54Z

assets/src/components/form/select/build-options.js

+	map = DEFAULT_MODEL_OPTIONS_MAP,
+) => {
+	const MAP_FOR_MODEL = map[ modelName ] ? map[ modelName ] : false;
+	const generatedOptions = entities && modelName && MAP_FOR_MODEL ?


Don't think we need && modelName here since it is not actually used anywhere in this block of code other than the previous line.
If modelName is not provided, then MAP_FOR_MODEL will resolve to false, therefore checking for modelName is a duplication of logic.

ya agreed, I'll fix that.

tn3rb · 2018-06-11T16:19:18Z

assets/src/components/form/select/build-options.js

+			label: addAllOptionLabel,
+			value: OPTION_SELECT_ALL,
+		} );
+	}


I think the previous ternary conditional should be an if else and this if conditional should be inside of that if block.

Why?

Because as is, if the selected model has no entities returned for the current query, then this conditional will still prepend the options list with a "Select All" option (assuming a label is set), which imho does not make sense.

"Select All" ???
Select All of what?
All of nothing?

IMHO, if there are no entities, then this should just return a completely empty list.

agreed. fixing.

tn3rb · 2018-06-11T16:21:03Z

assets/src/components/form/select/default-select-configuration.js

+		PropTypes.object,
+		PropTypes.array,
+	] ),
+};


this is awesome... but what about data attributes???

how would one add something like data-some-info="123456" to this input?

The properties this comment is attached to are all props used by the react-select component. Our <ModelSelect /> component just passes through any set props specifically for the react-select component via the props.selectConfiguration (which then gets added to the state inModelSelect). So, adding data-* attributes (if necessary) would need to be implemented via the react-select component. If we encounter a place where we absolutely need them, we can likely use the components prop to override one of the react select components to add the data attributes.

tn3rb · 2018-06-11T16:53:19Z

assets/src/components/form/select/model-select.js

+			...selectConfiguration,
+			...updated,
+		};
+	}


According to the React documentation:
https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops
and this blog post from 4 days ago:
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

derived state should be used sparingly

Our usage here, seems to fall under this use case:

If you need to perform a side effect (for example, data fetching or an animation) in response to a change in props, use componentDidUpdate lifecycle instead.

I think the information in that "You Probably Don't Need Derived State" post should be seriously considered and its alternate suggestions understood completely before moving forward with this implementation. I understand that that blog post did not exist when you first started developing this component so you can not be faulted in any way for using that approach (makes me almost think they wrote it just for you!).

I originally intended to link to the "You probably don't need derived state" in my original description for this pull but linked to a different (but still related) post by mistake. For context, here's what I wrote in the description for the pull:

I think I may need to tweak how I'm using derivedStateFromProps in ModelSelect yet. Basically I should only be changing state if there is a change detected for the incoming props with the current state (see example here). I can probably utilize memoize a bit in the some of the helper functions as well. This can be something done after this pull is merged and we are using it in blocks (so we can measure improvements).

We talked about this in slack but to summarize:

I'm aware of the potential issues.

I'm not convinced (yet) that we're doing things wrong (or in a way that is problematic).

I fully intend on investigating further to see if we can do what's needed without using getDerivedStateFromProps but think this is something that will be easier to investigate in actual implementations of this component. So I'd prefer to get this merged in then create another pull to test with and try different potential solutions.

tn3rb · 2018-06-11T16:57:47Z

assets/src/components/form/select/model-selects/datetime.js

+		selectConfiguration: {
+			loadingMessage: () => __( 'Retrieving Datetimes.', 'event_espresso' ),
+			noOptionsMessage: () => __(
+				'There are no datetimes available to select from.',


imho something totally dead simple like: __( 'No Datetimes.', 'event_espresso' ), will work better in more use cases because it will not expand the width of the input as much as this label.

From my testing it doesn't expand the width of the input, but I like it more concise anyways so will fix.

tn3rb · 2018-06-11T17:09:16Z

assets/src/data/model/datetime/index.js

+export const getQueryString = ( queryData = {} ) => {
+	queryData = { ...defaultQueryData.queryData, ...queryData };
+	return baseGetQueryString( queryData, whereConditions, mapOrderBy );
+};


missing newline at end of file

tn3rb · 2018-06-11T17:10:42Z

assets/src/data/model/event/index.js

+			'ticket_start',
+			'ticket_end',
+		] ),
+		order: PropTypes.oneOf( [ 'asc', 'desc' ] ),


same question as with the datetimes model... allow for caps, convert case, or use constants ?

tn3rb · 2018-06-11T17:11:21Z

assets/src/data/model/datetime/index.js

+/**
+ * Default attributes for this model
+ * @type {{attributes: {limit: number, orderBy: string, order: string,
+ *   showExpired: boolean}}}


maybe this could be formatted better?

ya will do. This was done automatically by PhpStorm but doesn't look like there is a way to fix the auto-formatting for this so manual fixes will be necessary.

tn3rb · 2018-06-11T17:11:36Z

assets/src/data/model/event/index.js

+/**
+ * Default attributes for this model
+ * @type {{attributes: {limit: number, orderBy: string, order: string,
+ *   showExpired: boolean}}}


better formatting ?

tn3rb · 2018-06-11T17:23:10Z

assets/src/data/model/event/index.js

+			'&where[Datetime.DTT_EVT_end][]=' +
+			moment().month( month ).endOf( 'month' ).local().format() );
+	}
+	return where.join( '&' );


If a component or block requires a where condition that is not here, then in your opinion should we:

add the new condition and corresponding prop to this method and everywhere else it is required

override this method (or another) and simply append the additional where condition to the query string

One of the reasons I extracted these into their own files is so that they could be considered the "authority" for parsing where conditions for that model. So for now, the option would be "add the new condition and corresponding prop to this method and everywhere else it is required".

However, as we see patterns emerge, we may find that we can autogenerate some of these a bit (or at least start with a common base).

nerrad added type:feature-request 🙏 category:gutenberg category:assets labels Jun 8, 2018

nerrad assigned tn3rb Jun 8, 2018

nerrad requested a review from tn3rb June 8, 2018 19:44

nerrad added 20 commits June 8, 2018 15:45

install react-select

ccee061

add utility for retrieving current available modelNames

86de3ac

This is populated via eejs.data from the server.

add an abstracted base method for generating queryString

2fda45a

extract model related methods to their own class

5be2598

add additional exports

d300c47

implement new model-select component

c80accc

This uses the react-select component

implement new EventSelect that wraps model-select

1a3c946

fix incorrect model map index

9358e8b

add newline at end of file.

b945ce9

remove unnecessary component wrapper and include defaults in props fo…

b42b60d

…r react-select component

remove unnecessary component wrapper and preserve modelName via state

4f1897f

fix bug with Object.assign and swith to spread syntax

f4d6802

test all the things

edd080c

bring in changes after testing against eventattendees branch

c91de76

update build files

44e7ef7

remove old event select

2c55fe9

fix Select Label

975eb8f

update snapshot due to changes

bff2d17

fix all tests

ba590b7

linting fixes

8aa3f5d

nerrad force-pushed the Gutenberg/implement-react-select-2 branch from 0e1d753 to 8aa3f5d Compare June 8, 2018 19:52

ee-duplicate-ticket-detector bot mentioned this pull request Jun 9, 2018

Implement React Strict Mode for our components #516

Closed

nerrad assigned nerrad and unassigned tn3rb Jun 11, 2018

nerrad removed the request for review from tn3rb June 11, 2018 13:26

nerrad added 4 commits June 11, 2018 10:48

convert datetime select to new component

4426015

export build-options for components

3c32f7e

fix labels and proptypes definition

d76cd98

add tests for new component (update snapshot too)

e742b40

nerrad requested a review from tn3rb June 11, 2018 15:32

nerrad assigned tn3rb and unassigned nerrad Jun 11, 2018

fix linting errors

812bcd8

tn3rb reviewed Jun 11, 2018

View reviewed changes

tn3rb assigned nerrad and unassigned tn3rb Jun 11, 2018

tn3rb added the status:review label Jun 11, 2018

nerrad added 4 commits June 11, 2018 14:43

fix conditionals

9d3f685

make strings more concise

7944369

add constants for query order values and implement

7c8e245

fix tests and snapshots

0071aec

nerrad requested a review from tn3rb June 11, 2018 18:46

nerrad assigned tn3rb and unassigned nerrad Jun 11, 2018

tn3rb approved these changes Jun 11, 2018

View reviewed changes

tn3rb assigned nerrad and unassigned tn3rb Jun 11, 2018

nerrad merged commit 26c214f into Gutenberg/master Jun 11, 2018

nerrad deleted the Gutenberg/implement-react-select-2 branch June 11, 2018 19:15

nerrad mentioned this pull request Jun 11, 2018

Investigate usage of getDerivedStateFromProps. #519

Closed

tn3rb removed the status:review label Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement react-select for our select input components #515

Implement react-select for our select input components #515

nerrad commented Jun 8, 2018 •

edited

Loading

tn3rb left a comment

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

tn3rb Jun 11, 2018

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

tn3rb Jun 11, 2018

tn3rb Jun 11, 2018

nerrad Jun 11, 2018

Implement react-select for our select input components #515

Implement react-select for our select input components #515

Conversation

nerrad commented Jun 8, 2018 • edited Loading

Problem this Pull Request solves

Abstracting out things specific to model related data interaction into the data/model folder.

Changing folder structure to more clearly reflect the component context.

Building a base ModelSelect component with an accompanying withSelect HOC wrapping it.

What remains to do?

Important things to note for converting over older selects to this new component:

OnChange callback

How has this been tested

Checklist

tn3rb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derived state should be used sparingly

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad commented Jun 8, 2018 •

edited

Loading

Abstracting out things specific to model related data interaction into the `data/model` folder.

Building a base `ModelSelect` component with an accompanying `withSelect` HOC wrapping it.

`OnChange` callback