Add ReferenceManyInput #484

kimkha · 2017-03-20T15:27:29Z

I finished issue #45 . Please review.

Merge

# Conflicts: # src/mui/input/ArrayInput.js

# Conflicts: # src/mui/input/SelectManyInput.js

fzaninotto · 2017-03-20T16:12:54Z

Can you please PR against the next branch?

kimkha · 2017-03-21T00:51:36Z

OK.

kimkha · 2017-03-21T00:53:17Z

DONE. But look like no new travis build against next branch, right?

kimkha · 2017-03-22T03:49:18Z

@fzaninotto Is it OK?

fzaninotto

Super encouraging, looking forward to merging this one! Your PR needs a few changes before that.

fzaninotto · 2017-03-22T19:05:00Z

src/mui/input/ReferenceManyInput.js

@@ -0,0 +1,222 @@
+import React, {Component, PropTypes} from 'react';


CS: please add spaces inside brackets. {Component, PropTypes} => { Component, PropTypes }

This happens in many places in this PR.

:D The IntelliJ auto formatting...

fzaninotto · 2017-03-22T19:08:05Z

src/mui/input/ReferenceManyInput.js

+const noFilter = () => true;
+
+/**
+ * An Input component for choosing many reference records. Useful for 'hasMany' relationship.


It would be nice to show the expected value in the record (an array of ids)

You mean, I need to provide detailed comment?

fzaninotto · 2017-03-22T19:10:45Z

src/mui/input/SelectManyInput.js

+ *
+ * Pass possible options as an array of objects in the 'choices' attribute.
+ * @example
+ * const choices = [ 'Male', 'Female' ];


I don't agree with the addition of this type prop. It complicates things IMO. Keep to the same API as SelectInput and remove the type prop.

fzaninotto · 2017-03-22T19:12:43Z

src/mui/input/ReferenceManyInput.js

+        const {pagination, sort, filter} = this.params;
+        const ids = input.value;
+        if (ids && Array.isArray(ids)) {
+            this.props.crudGetMany(reference, ids);


you should probably throw an explicit error if ids is not an array.

fzaninotto · 2017-03-22T19:14:08Z

src/mui/input/ReferenceManyInput.js

+ *     <SelectManyInput type="object" optionText="title" />
+ * </ReferenceManyInput>
+ */
+export class ReferenceManyInput extends Component {


my 2c: as there is a lot of common code with ReferenceInput, why don't you extend ReferenceInput instead of Component?

I will try to do so... :)

fzaninotto · 2017-03-22T19:16:51Z

src/mui/input/SelectManyInput.js

+export class SelectManyInput extends Component {
+    // Comment input onBlur because SelectManyInput don't pass correct values on this event
+    handleBlur = (eventOrValue) => {
+        // this.props.onBlur(eventOrValue);


commented out code

fzaninotto · 2017-03-22T19:18:02Z

src/mui/input/SelectManyInput.js

+ * @see https://github.com/TeamWertarbyte/material-ui-chip-input
+ */
+export class SelectManyInput extends Component {
+    // Comment input onBlur because SelectManyInput don't pass correct values on this event


I don't understand, can you elaborate? Not catching onBlur or onFocus from the child component will probably break redux-form validation

fzaninotto · 2017-03-22T19:18:15Z

src/mui/input/SelectManyInput.js

+    };
+    // Comment input onFocus because SelectManyInput don't pass correct values on this event
+    handleFocus = (event) => {
+        // this.props.onFocus(event);


same, please don't commit commented out code

fzaninotto · 2017-03-22T19:19:05Z

src/mui/input/SelectManyInput.js

+
+    handleChange = (eventOrValue) => {
+        if (this.props.type == 'object') {
+            eventOrValue = this.extractIds(eventOrValue);


don't modify a parameter of the function. Use a new const instead

fzaninotto · 2017-03-22T19:19:59Z

src/mui/input/SelectManyInput.js

+            type,
+        } = this.props;
+
+        // Always use uncontrolled mode


I don't understand. On the contrary, we should use controlled mode, shouldn't we?

fzaninotto · 2017-03-27T12:10:10Z

Any updates on this one?

kimkha · 2017-03-27T14:29:31Z

Sorry, I'm on vacation now, I will back in 2 more days... :)

DjLeChuck · 2017-03-31T15:31:03Z

I'm very interested in this component so I try to implement it with your code.

Some points I see (I'm new with redux-form/admin-on-rest so maybe I'm wrong!):

ReferenceManyInput
- crudGetOne should be removed from ReferenceManyInput props as it's not used
SelectManyInput
- You could (and should I guess) use the controlled mode. Using onRequestAdd and onRequestDelete instead of onChange works
- onBlur and onFocus are receiving the <input /> but his value is empty. It looks like this comment: Fix behaviour of onBlur and add an add-on-blur example to storybook TeamWertarbyte/material-ui-chip-input#70 (comment) but the problem was solved so I don't know what's wrong.

FYI, my onRequestAdd and onRequestDelete:

    handleAdd = (newValue) => {
      const { input: { value, onChange } } = this.props;

      onChange([...value, ...[newValue.id]]);
    };

    handleDelete = (newValue) => {
      const { input: { value, onChange } } = this.props;

      onChange(value.filter(v => (v != newValue)));
    };

EDIT:
I don't know why but if I don't pass onBlur, all my chips disappear when I loose focus. There is a default behaviour about onBlur and redux-form?

EDIT2:
Calling onBlur and onFocus from props works:

    <ChipInput
        onBlur={() => this.props.onBlur(defaultValue)}
        onFocus={() => this.props.onFocus(defaultValue)}
    />

DjLeChuck · 2017-04-03T07:45:06Z

In ReferenceManyInput::mapStateToProps, you should call getPossibleReferences even if there is no referenceIds otherwise the autocomplete list will not show up when the field is empty.

function mapStateToProps(state, props) {
    const referenceIds = props.input.value;
    const referenceRecords = [];
    let matchingReferences = [];
    const data = state.admin[props.reference].data;

    if (!referenceIds.length) {
        matchingReferences = getPossibleReferences(state, referenceSource(props.resource, props.source), props.reference);
    } else {
        for (let i = 0; i < referenceIds.length; i++) {
            if (data[referenceIds[i]]) {
                referenceRecords.push(data[referenceIds[i]]);
                const possibleReferences = getPossibleReferences(state, referenceSource(props.resource, props.source), props.reference, referenceIds[i]);
                matchingReferences = Object.assign(matchingReferences, possibleReferences);
            }
        }
    }

    return {
        referenceRecords: referenceRecords,
        matchingReferences: matchingReferences,
    }
}

In SelectManyInput, I have add onUpdateInput={this.props.setFilter} on <ChipInput /> so the data are fetch when the user's input change.

However, the setFilter function is not good for me. The received value is a string (the search text) and it's compared to this.params.filter which is an object so the condition is always false and data are always fetch.

Further, when the function is called, the default filters are erased by filterToQuery. For example my first call:

includeDisabled:false
fields:title
sort:title
limit:25
skip:0

My second call:

q:apprends

I know you use the same code as ReferenceInput and I can override filterToQuery but it's a strange behaviour IMO. Why not let the default filters and only add the search one? Maybe @fzaninotto you can explain me your thinking behind this usage?

fzaninotto · 2017-04-05T10:30:48Z

any ETA on this one? We'll be releasing 1.0 within a week, and I'd love to see it merged.

…rmelab/admin-on-rest/pull/484)

…/marmelab/admin-on-rest/pull/484)

kimkha · 2017-04-06T03:40:00Z

ReferenceManyInput look same with ReferenceInput, but from recommendation of FB, we shouldn't use inheritance way (see https://facebook.github.io/react/docs/composition-vs-inheritance.html#so-what-about-inheritance)

…/marmelab/admin-on-rest/pull/484)

kimkha · 2017-04-06T03:48:17Z

@fzaninotto Please check again! 😄

fzaninotto

Still some work to do. Also, could you document the two fields in docs/Inputs.md?

fzaninotto · 2017-04-06T07:03:42Z

src/mui/input/ReferenceManyInput.js

+ *
+ * @example
+ * {
+ *    postIds: [ "1", "23", "4" ]


The following example (and the ones below) uses post_id and this one postIds. Please make them consistent. I think it should be post_ids everywhere.

fzaninotto · 2017-04-06T07:06:21Z

src/mui/input/ReferenceManyInput.js

+            choices: matchingReferences,
+            basePath,
+            onChange,
+            filter: noFilter, // for AutocompleteInput


fzaninotto · 2017-04-06T07:07:31Z

src/mui/input/ReferenceManyInput.js

+    filter: {},
+    filterToQuery: searchText => ({ q: searchText }),
+    matchingReferences: [],
+    meta: {},


I've removed this one from other components because it has side effects, please remove it here, too.

you mean the filter and filterToQuery?

no, I mean meta. Use other components as models.

fzaninotto · 2017-04-06T07:08:28Z

src/mui/input/ReferenceManyInput.js

+    if (!referenceIds.length) {
+        matchingReferences = getPossibleReferences(state, referenceSource(props.resource, props.source), props.reference);
+    } else {
+        for (let i = 0; i < referenceIds.length; i++) {


This should be placed in a selector, otherwise it's executed for every global state change.

Ah, I see... But what should I do?

create a selector in the reducer - there are others like it (see https://github.com/marmelab/admin-on-rest/blob/master/src/reducer/references/oneToMany.js#L19-L29)

fzaninotto · 2017-04-06T07:08:53Z

src/mui/input/SelectManyInput.js

+import FieldTitle from '../../util/FieldTitle';
+
+/**
+ * An Input component for a array


s/for a array/for an array/

fzaninotto · 2017-04-06T07:09:24Z

src/mui/input/SelectManyInput.js

+ * An Input component for a array
+ *
+ * @example
+ * <SelectManyInput source="first_name" />


bad example. How can first_name be an array? Same for the rest of the comments for that component.

fzaninotto · 2017-04-06T07:10:27Z

src/mui/input/SelectManyInput.js

+    };
+
+    extractIds = (eventOrValue) => {
+        let value = eventOrValue;


make it a const by using a ternary expression.

fzaninotto · 2017-04-06T07:12:39Z

src/mui/input/SelectManyInput.js

+            return this.props.choices.filter((o) => ids.indexOf(o[this.props.optionValue]) >= 0);
+        } else {
+            return ids.map((id) => {
+                let o = {};


better return an object literal directly:

return ids.map((id) => ({ [this.props.optionValue]: id, [this.props.optionText]: id, }))

Also, how come you use the id both for value and text?

I will change the variable name... 👍

fzaninotto · 2017-04-06T07:13:11Z

src/mui/input/SelectManyInput.js

+
+    handleChange = (eventOrValue) => {
+        var extracted = this.extractIds(eventOrValue);
+        this.props.onChange(extracted);


what's this onChange for? Same for the other custom handlers (onBlur, etc): they don't exist in other components, so it doesn't make sense to add them just for that component

I'm not sure... I copied from TextInput, what's it for?

From the doc:

Some other props are progressively implemented. The <TextInput /> and <NumberInput /> inputs also accept following props:
onBlur: [...]
onChange: [...]
onFocus: [...]

my bad, you're right

fzaninotto · 2017-04-06T07:47:50Z

src/mui/input/SelectManyInput.js

+        } = this.props;
+
+        // Convert the name of fields in choices
+        options['dataSourceConfig'] = {


please use dot notation. same for dataSource

…https://github.com/marmelab/admin-on-rest/pull/484)

fzaninotto

We're getting very close. To do before merge:

Move the logic from mapStateToProps to a selector
Add documentation for the 2 new inputs

fzaninotto · 2017-04-07T06:46:01Z

src/mui/input/SelectManyInput.js

+
+        if (this.props.choices && this.props.choices.length > 0) {
+            return this.props.choices.filter((o) => values.indexOf(o[this.props.optionValue]) >= 0);
+        } else {


No need for an else after a return

DjLeChuck · 2017-04-07T06:53:46Z

Please, what about the onUpdateInput={this.props.setFilter} property on <ChipInput />?

Actualy, only one request is made to load possible values. If the user type text it only filter loaded values and not make aditionnal requests. It's the good behaviour @fzaninotto ?

fzaninotto · 2017-04-07T06:59:08Z

I don't know how <ChipInput> is supposed to work. But if there is a problem in your tests, please advise @kimkha about it.

DjLeChuck · 2017-04-07T07:03:49Z

Sorry, my question is not about <ChipInput /> but how the component should work. My question can be the same with the component ReferenceInput. When the user type text in the field, this should only filter loaded documents or it should do additional request with the searches term (using filterToQuery)?

fzaninotto · 2017-04-07T07:12:43Z

The ChipInput should pass the new search text up to ReferenceManyInput, which should issue a new request.

kimkha · 2017-04-07T07:17:49Z

So, I think that we should remove filter on SelectManyInput, right?

DjLeChuck · 2017-04-07T07:20:26Z

Actually it's on <ReferenceManyInput />, not <SelectManyInput /> 😄

kimkha · 2017-04-07T07:30:13Z

Ah, I want to make ReferenceManyInput to handle all filter request, not ChipInput. Because ChipInput is managed by SelectManyInput, but SelectManyInput can use like other inputs (put directly on <Edit /> or <Create />). So, handling filter in SelectManyInput (I mean passing filter from ReferenceManyInput) will make something messed.

And, later, I think we also support CheckboxGroupInput as a child of ReferenceManyInput...

DjLeChuck · 2017-04-07T07:37:44Z

Yes, ReferenceManyInput should handle filtering that's what @fzaninotto said! 😄

But SelectManyInput should tell to ReferenceManyInput to launch a new request when the user type in the input. That's the aim of onUpdateInput property (the doc of ChipInput says Callback function that is called when the input changes (useful for auto complete).)

kimkha · 2017-04-07T07:47:30Z

Ah, sorry because I didn't understand what you mean...

I will implement it soon. :D

…https://github.com/marmelab/admin-on-rest/pull/484)

kimkha · 2017-04-07T23:23:22Z

@DjLeChuck Done for onUpdateInput

kimkha · 2017-04-07T23:35:21Z

@fzaninotto About map state, let me explain why I need 2 params:

referenceRecords is used to display current selection (especially on Edit page)
matchingReferences is used to filter the input results (auto complete)

But to make it display correctly, I must join them to a single param (matchingReferences in my case), so that I can send matchingReferences to ChipInput to display both (the current selection and the chosen list).

If we want to move to a reducer selector, we must find out better solution. What should I do for that?

…https://github.com/marmelab/admin-on-rest/pull/484)

DjLeChuck · 2017-04-13T08:10:26Z

I have a strange behaviour and I don't know if it's my fault or not.
Using this code in Edit, the object has 2 themes:

<ReferenceManyInput label="Thèmes" source="themes" reference="themes" allowEmpty>
    <SelectManyInput optionText="label" options={themesSelectStyle} />
</ReferenceManyInput>

If I come from the listing: No Chips
If I hit directly the URL (or reload the page): Chips but with ID, not the label
If I remove allowEmpty and come from the listing: Chips with label
If I remove allowEmpty hit directly the URL (or reload the page): Only the first Chip with label

fzaninotto · 2017-04-25T11:54:55Z

With all the rebases and merges it was hard to keep track of the code, so I've squashed your commits (keeping you as the author) and started a new PR (#597) which I'll finish.

kimkha · 2017-04-25T13:36:09Z

Many thanks... :)

kimkha added 9 commits March 15, 2017 17:22

Merge pull request #1 from marmelab/master

362b441

Merge

marmelab#45: Use chip input for ArrayInput

5c935ed

marmelab#45: Allow choice in ArrayInput, and data may be an object.

2e96187

marmelab#45: Use ReferenceInput with ArrayInput for MultiReference

b7924be

Merge remote-tracking branch 'origin/master'

cd0a8d4

# Conflicts: # src/mui/input/ArrayInput.js

marmelab#45: Fix comment in ArrayInput.

7f4c4ae

marmelab#45: Rename to ReferenceManyInput and SelectManyInput

48a8f38

Merge remote-tracking branch 'origin/master'

fa12bb7

# Conflicts: # src/mui/input/SelectManyInput.js

marmelab#45: Revert ReferenceInput

cd51eef

kimkha changed the base branch from master to next March 21, 2017 00:51

fzaninotto requested changes Mar 22, 2017

View reviewed changes

kimkha added 2 commits April 1, 2017 20:38

marmelab#45: Format space

e1c71aa

marmelab#45: Update comments

6c0309c

kimkha added 2 commits April 4, 2017 23:26

marmelab#45: Remove type in SelectManyInput

ba331f0

marmelab#45: Remove type in SelectManyInput

e3e4ea1

kimkha added 2 commits April 5, 2017 22:06

marmelab#45: Refactor SelectManyInput (based on https://github.com/ma…

b14b7c2

…rmelab/admin-on-rest/pull/484)

marmelab#45: Refactor ReferenceManyInput (based on https://github.com…

8c6d3e3

…/marmelab/admin-on-rest/pull/484)

kimkha added 2 commits April 6, 2017 10:40

marmelab#45: Refactor ReferenceManyInput (based on https://github.com…

bc812e8

…/marmelab/admin-on-rest/pull/484)

Merge branch 'next' into master

e0e5ad5

fzaninotto requested changes Apr 6, 2017

View reviewed changes

kimkha added 3 commits April 6, 2017 16:33

marmelab#45: Refactor ReferenceManyInput and SelectManyInput (based on …

8a0629a

…https://github.com/marmelab/admin-on-rest/pull/484)

Merge remote-tracking branch 'origin/master'

61a5c7f

marmelab#45: Refactor ReferenceManyInput and SelectManyInput (based on …

b024fb7

…https://github.com/marmelab/admin-on-rest/pull/484)

fzaninotto requested changes Apr 7, 2017

View reviewed changes

marmelab#45: Refactor ReferenceManyInput and SelectManyInput (based on …

cd041da

…https://github.com/marmelab/admin-on-rest/pull/484)

kimkha added 2 commits April 8, 2017 06:36

marmelab#45: Refactor ReferenceManyInput and SelectManyInput (based on …

1df6c34

…https://github.com/marmelab/admin-on-rest/pull/484)

marmelab#45: Refactor ReferenceManyInput and SelectManyInput (based on …

844ed35

…https://github.com/marmelab/admin-on-rest/pull/484)

Merge branch 'next' into master

6eea413

fzaninotto mentioned this pull request Apr 25, 2017

[RFR] Add Reference many input #597

Merged

7 tasks

fzaninotto closed this Apr 25, 2017

		@@ -0,0 +1,222 @@
		import React, {Component, PropTypes} from 'react';

Add ReferenceManyInput #484

Add ReferenceManyInput #484

Conversation

kimkha commented Mar 20, 2017

fzaninotto commented Mar 20, 2017

kimkha commented Mar 21, 2017

kimkha commented Mar 21, 2017

kimkha commented Mar 22, 2017

fzaninotto 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

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

fzaninotto commented Mar 27, 2017

kimkha commented Mar 27, 2017

DjLeChuck commented Mar 31, 2017 • edited

DjLeChuck commented Apr 3, 2017

fzaninotto commented Apr 5, 2017

kimkha commented Apr 6, 2017

kimkha commented Apr 6, 2017

fzaninotto 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

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

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

fzaninotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DjLeChuck commented Apr 7, 2017

fzaninotto commented Apr 7, 2017

DjLeChuck commented Apr 7, 2017

fzaninotto commented Apr 7, 2017

kimkha commented Apr 7, 2017

DjLeChuck commented Apr 7, 2017

kimkha commented Apr 7, 2017 • edited

DjLeChuck commented Apr 7, 2017 • edited

kimkha commented Apr 7, 2017

kimkha commented Apr 7, 2017

kimkha commented Apr 7, 2017

DjLeChuck commented Apr 13, 2017 • edited

fzaninotto commented Apr 25, 2017

kimkha commented Apr 25, 2017

DjLeChuck commented Mar 31, 2017 •

edited

kimkha commented Apr 7, 2017 •

edited

DjLeChuck commented Apr 7, 2017 •

edited

DjLeChuck commented Apr 13, 2017 •

edited