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(relation-widget): Allow the relation widget to target file collections #3717

Closed
wants to merge 7 commits into from

Conversation

d4rekanguok
Copy link
Contributor

@d4rekanguok d4rekanguok commented May 7, 2020

Summary

Hello everyone! This PR is a WIP and is meant to facilitate discussion, no hurt feeling if it's rejected.

A few challenges when targeting a file collection:

  • Beside the collection name, the widget will also need the file name, and the name of a field in that file (which is very likely a list widget.)
  • The widget needs to know if it's dealing with a file collection, a folder collection, or its own entry.
  • The widget have to be able to handle a simple array of string and potentially an array of object with different shape (in case of an array produced by a typed list widget).

For the first two, I think we could use dot notation in collection name:

# folder collection
- label: Featured Posts
  name: featuredPosts
  widget: relation
  collection: posts

# file collection
- label: Categories
  name: categories
  widget: relation
  # [collection].[fileName].[fieldName]
  collection: "settings.post.categories"

If collection contains dots, use query as usual; otherwise use loadEntry. Later on, when a user want to target a list widget in the same entry as the relation widget, I think it should be smart and use the new entry props.

Also, I found that query's results are stored in queryHits. I wonder if loadEntry results should be stored in a similar manner?

Please let me know if this is a valid approach!

Test plan

I've written 2 basic tests that covers a simple list & a list of items with the same object shape. Will need a few more.

closes #800

@d4rekanguok d4rekanguok requested a review from a team May 7, 2020 17:14
@erezrokah erezrokah self-requested a review May 11, 2020 14:12
@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label May 11, 2020
@d4rekanguok
Copy link
Contributor Author

I fleshed this out a bit more today, adding support for targeting list being nested inside an object! I'm rather fond of overloading collection field, since it allows us to easily differ between file & folder collections. Later on if we decide to allow user to target fields within the same entry, perhaps user can specify some thing like this: collection: "self.fieldName".

I also added a category field to the demo site to test this out visually, and found an issue where loaded options are not being persisted. I think it may have to do with the fact that in a folder-targeting relation widget, the options loaded via query are persisted in the redux store as queryHits[forID], but there's no such thing for loadEntry.

In my custom widget, I persist loaded options in the widget's local state, but it doesn't seem to work here. It's also likely something to do with my code, will look more into it.

@d4rekanguok
Copy link
Contributor Author

d4rekanguok commented May 13, 2020

Hhhm I'm a bit stuck here. Intuitively I think it's best to replicate the same structure from folder relation, but even if I want to do so, the function loadEntry passed to widget is no longer a redux action and thus can't send the payload to the global store the same way query can (unless we change that back.)

Another approach I can think of, is to do loadEntry/query upfront in componentDidMount and doesn't rely on value stored in queryHits for the first render.

@d4rekanguok d4rekanguok marked this pull request as draft May 13, 2020 07:27
@erezrokah
Copy link
Contributor

erezrokah commented May 13, 2020

Hi @d4rekanguok, was meaning to comment on this soon (haven't had the time to test my thoughts yet), but I think you can dump loadEntry and use

if (field.has('file')) {
    hits = hits.filter(hit => hit.slug === field.get('file'));
}

Disadvantage it that the CMS will still retrieve all files in the referenced collection, but I think it is a minor issue considering the alternative.

I would actually not overload the collection field since it introduces a new concept. Having an additional file field might be easier to grasp, and using the current valueField to reference the file field might work. Haven't thought of a good way to reference lists yet (maybe categories.*.name).

@d4rekanguok
Copy link
Contributor Author

Thanks @erezrokah, that makes sense! I'll explore the approach you mentioned.

@erezrokah
Copy link
Contributor

Replaced by #3754

@erezrokah erezrokah closed this May 18, 2020
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
2 participants