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

[RFR] Add Reference many input #597

Merged
merged 19 commits into from May 22, 2017
Merged

[RFR] Add Reference many input #597

merged 19 commits into from May 22, 2017

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Apr 25, 2017

  • Rebase and squash the code from Add ReferenceManyInput #484
  • Rename components to Array, to be consistent with feat: add ReferenceArrayField #596
  • Add <ReferenceArrayInput>
  • Add <SelectArrayInput>
  • Use the new components in the blog example (for tags)
  • Add documentation
  • Translate options by default on <SelectArrayInput>, except when in a <ReferenceArrayInput>

Supersedes #484, closes #45

@leesei
Copy link
Contributor

leesei commented Apr 25, 2017

One question on Many vs Array, will AOR use Many for field filters and Array for id filters?

@fzaninotto
Copy link
Member Author

@leesei I don't understand your question

@leesei
Copy link
Contributor

leesei commented Apr 25, 2017

I see the task "Rename components to Array", so was wondering AOR's rule for using Many vs Array.

@fzaninotto
Copy link
Member Author

Here it's Array because we expect the value to be an array. But in the perpective of many-to-many relationships via a hash table (not yet implemented), then it'll be a Many.

@fzaninotto fzaninotto changed the title [WIP] Add Reference many input [RFR] Add Reference many input Apr 25, 2017
@fzaninotto
Copy link
Member Author

Switching to Ready For Review. @kimkha, as you can see, there was a lot to do to make your PR mergeable.

@kimkha
Copy link
Contributor

kimkha commented Apr 26, 2017

You're awesome! 😍

So, how about the test?

@fzaninotto
Copy link
Member Author

I should add somewhere that <CheckboxGroupInput> is also available for this component.

Copy link
Contributor

@jpetitcolas jpetitcolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome!

Yet, I don't see any tests. :/

docs/Inputs.md Outdated
</ReferenceArrayInput>
```

**Tip**: `allowEmpty` is set by default for all Input components children of the `<Filter>` component:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/component:/component/

@blindsteal
Copy link

Regarding your FIXME with the ids instead of values on first draw: I've worked on this problem before, it is due to the datasource not being populated at that point. The solution is calling resolveValues again when it does get populated.

Here is my code, few things to note:

  • getChoices has been renamed to formatChoices, since it now takes the choices as an argument. Don't forget to replace in render.
  • Don't ask me how I arrived at the guard condition for componentWillReceiveProps, I don't remember but this is the one that worked for all cases (coming from the list view, directly loading the edit view, various input lengths etc).
    resolveValues = (values, choices) => {
        if (!values || !Array.isArray(values)) {
            throw Error('Value of SelectArrayInput should be an array');
        }

        if (choices && choices.length > 0) {
            return this.formatChoices(choices).filter(choice => values.includes(choice.value));
        }
        return values.map(value => ({
            value, text: value,
        }));
    };

    formatChoices = (choices) => {
        const {
            optionText,
            optionValue,
            translate,
            translateChoice,
        } = this.props;
        return choices.map(choice => ({
            value: choice[optionValue],
            text: translateChoice ? translate(choice[optionText], { _: choice[optionText] }) : choice[optionText],
        }));
    }

    componentWillReceiveProps = nextProps => {
      if (
        this.props.choices.length !== nextProps.choices.length ||
        this.props.input.value.length < nextProps.input.value.length
      ) {
        this.setState({
          values: this.resolveValues(nextProps.input.value, nextProps.choices)
        });
      }
    };

@fzaninotto fzaninotto added this to the 1.1.0 milestone May 2, 2017
@blindsteal
Copy link

Apparently I did not catch every case... We also need to resolve in componentWillMount (in case choices and input are already populated but the constructor is not called... i.e. when visiting the edit page for the second time without reloading).

  componentWillMount = () => {
    this.setState({
      values: this.resolveValues(this.props.input.value, this.props.choices)
    });
  }

@fzaninotto
Copy link
Member Author

Now really green and ready for merge

Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work !

export const getPossibleReferences = (state, referenceSource, reference, selectedIds = []) => {
const possibleValues = state.admin.references.possibleValues[referenceSource] || [];
if (selectedIds.length !== 0) {
selectedIds.forEach(id => possibleValues.includes(id) || possibleValues.unshift(id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you mutating the current state here ? It would probably be safer to initialize possibleValues from state with Array.from

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right - even if that was the case before that PR, too.

@djhi djhi merged commit 2a3393c into next May 22, 2017
@djhi djhi deleted the reference_many_input branch May 22, 2017 07:13
@fzaninotto
Copy link
Member Author

Thanks @kimkha!

Granddevv referenced this pull request in Granddevv/React-Admin-panel-restful-apis- Jan 15, 2018
coderbag referenced this pull request in coderbag/admin-on-rest Jan 20, 2018
@rchoffar
Copy link

rchoffar commented Apr 27, 2018

Has this input method been deprecated ? @fzaninotto

@fzaninotto
Copy link
Member Author

@fr0Xy <ReferenceArrayInput> and <SelectArrayInput> are still there. What do you mean?

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

Successfully merging this pull request may close these issues.

None yet

8 participants