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

feat(widget-relation): target file collections #3754

Merged
merged 7 commits into from
May 19, 2020

Conversation

d4rekanguok
Copy link
Contributor

@d4rekanguok d4rekanguok commented May 13, 2020

(This PR continues the discussion in #3717 & may replace it)

Summary

Allowing relation widget to target file collections, using query as suggested by @erezrokah

Test plan

A picture of a cute animal (not mandatory but encouraged)

closes #800
closes #3717

@d4rekanguok d4rekanguok requested a review from a team May 13, 2020 15:27
@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label May 13, 2020
@d4rekanguok d4rekanguok marked this pull request as draft May 13, 2020 15:28
@erezrokah erezrokah self-requested a review May 13, 2020 15:38
@d4rekanguok
Copy link
Contributor Author

@erezrokah Using query as suggested, I was able to get it working more properly with less code modification vs. using loadEntry.

I planed to query all entries in a file collections then filter them by slug in parseHitOptions, but I think it wouldn't work without changing these lines to make extract optional:

https://github.com/netlify/netlify-cms/blob/6042383b9db696902f34636f12f44cbfea30562e/packages/netlify-cms-core/src/backend.ts#L428-L429

What I ended up doing is this:

    const file = fields.get('file');

    const queryPromise = file
      ? query(forID, collection, ['slug'], file)  // get a single entry with exact match
      : query(forID, collection, searchFieldsArray, term);

I'm also temporary using searchFields for the target field instead of valueField as suggested, since valueField can be used for object list. I think we may want to use a new field for this instead, since it's pretty different than searchFields for a folder collection (it shouldn't accept an array):

# target a simple list
  - label: Tags
    name: tags
    widget: relation
    collection: settings
    file: general
    searchFields: simpleList

# target an object list
  - label: Categories
    name: categories
    widget: relation
    collection: settings
    file: general
    searchFields: objectList
    valueField: id
    displayFields: '{{id}} | {{description}}'
   
# somewhere else...

collections:
  - name: settings
    files:
      - name: general
        fields:
          - { label: 'Tags', name: 'simpleList', widget: 'list' }
          - label: Categories, 
            name: objectList
            widget: list
            fields:
              - { label: 'id', name: 'id', widget: 'string' }
              - { label: 'desc', name: 'description', widget: 'string' }

Overall I prefer this over loadEntry, since it's much more inline with the current relation widget.

@@ -187,7 +205,8 @@ export default class RelationControl extends React.Component {
const isClearable = !field.get('required', true) || isMultiple;

const hits = queryHits.get(forID, []);
const options = this.allOptions || this.parseHitOptions(hits);
const options =
this.allOptions || this.parseHitOptions(hits, { isFile: Boolean(field.get('file')) });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah oops this is from a previous experiment, I'll delete this code

label: labelReturn,
};
});
let data = hits;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Variable naming here is aweful, but I have a hard time coming up with something better. Perhaps this whole block could be rewritten to be better

});
let data = hits;
if (field.get('file')) {
data = get(hits, `[0].data.${searchFields}`, []).map(item => ({ data: item }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand why the field that will be searched for in the file collection is assumed to be an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I should add an array check here, thanks for spotting this!

@erezrokah
Copy link
Contributor

@d4rekanguok, I rebased the branch and added a commit with the valueField approach, allowing wildcards.
For example:

collections:
  - name: posts
    label: Posts
    label_singular: 'Post'
    folder: content/posts
    create: true
    fields:
      - label: Title
        name: title
        widget: string
      - label: Relation
        name: relation
        widget: relation
        collection: files
        valueField: categories.*.id
        searchFields: [categories.*.name] # not really used
        displayFields: [categories.*.name]
        file: categories
  - name: files
    label: Files
    label_singular: 'File'
    files:
      - label: Categories
        name: categories
        file: _data/_cats.json
        fields:
          - label: Categories
            name: categories
            widget: list
            allow_add: true
            fields:
              - label: Name
                name: name
                widget: string
              - label: Id
                name: id
                widget: string

This keeps the same semantics for the fields, and allows nesting in a generic way.
WDYT?

@@ -128,24 +155,26 @@ export default class RelationControl extends React.Component {
parseHitOptions = hits => {
const { field } = this.props;
const valueField = field.get('valueField');
const displayField = field.get('displayFields') || field.get('valueField');
const displayField = field.get('displayFields') || List(field.get('valueField'));
Copy link
Contributor

@erezrokah erezrokah May 18, 2020

Choose a reason for hiding this comment

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

Makes sure displayField is always a list to avoid checking it later like https://github.com/netlify/netlify-cms/pull/3754/files#diff-1c6e2a499213935b7c1b04dbb4e1ca1cL135

@d4rekanguok
Copy link
Contributor Author

Neat, yes this looks great!

.join(' ');
const value = this.parseNestedFields(hit, valuesPaths[i]);
acc.push({ data: hit.data, value, label });
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this is really neat 👍

@erezrokah erezrokah merged commit 2f435f8 into master May 19, 2020
@erezrokah erezrokah deleted the feat/relation-file-hits branch May 19, 2020 07:18
vladdu pushed a commit to vladdu/netlify-cms that referenced this pull request Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relation doesn't work for collections defined within a file-based collection
3 participants