Improve relationships #49
Conversation
|
There was a problem hiding this comment.
@jesstelford some comments for you. Looks good but would be nice to confirm these things 👍
| {fields.map((field, index) => { | ||
| const { path } = field; | ||
|
|
||
| const isLoading = !item.hasOwnProperty(path); |
There was a problem hiding this comment.
What does isLoading mean in this context? not sure the conditions under which this could occur 😕
There was a problem hiding this comment.
Yeah, me either. But I was getting errors because item didn't have certain keys on the object, and that component was getting rendered multiple times in quick succession.
This was all occurring immediately after I selected to display a relationship field from the dropdown, so I think it's related to triggering the data fetching query - the data hasn't yet returned, but somewhere up the hierarchy, that field is included in what to render, so it errored.
| @@ -36,7 +36,7 @@ import ColumnSelect from './ColumnSelect'; | |||
| import FilterSelect from './FilterSelect'; | |||
| import SortSelect, { SortButton } from './SortSelect'; | |||
|
|
|||
| const getQueryArgs = args => { | |||
| const getQueryArgs = (args) => { | |||
There was a problem hiding this comment.
Not formatted by prettier
| getValue = data => { | ||
| const { many, path } = this.config; | ||
| return data[path] ? data[path] : many ? [] : null; | ||
| return data[path] ? data[path].id : many ? [] : null; |
There was a problem hiding this comment.
I think this would only work for single relationships, the logic needs to be a bit more sophisticated now:
return many ? data[path].map(i => i.id) : data[path] ? data[path].id : null;
As far as I can tell, for many relationships, GraphQL will always return an array here even if there are no related items, so that should be safe. But needs verification.
There was a problem hiding this comment.
Good catch!
Probably time to split it into if/else, and I think this way will work well:
if (!data[path]) {
return many ? [] : null;
}
return many ? data[path].map(i => i.id) : data[path].id;edit: Done in 12ba8f7
|
Turns out I need some of the code in this branch for the files fields, so I'm going to base my branch off of |
jesstelford
force-pushed
the
improve-relationships
branch
from
12ba8f7 to
03c808f
Compare
|
I've rebased |
jesstelford
force-pushed
the
improve-relationships
branch
from
03c808f to
dd3b8bc
Compare
|
Just noticed a bug with creating an item that contains a relationship. Fixed in c6479cc |
|
@jesstelford I'm still seeing some bad behaviour in the
This is what's happening for |
|
I shall fix that this morning 👍 |
|
Fixed 🎉 See the diff here: keystonejs/keystone-5@d487f091d207d1cf9b4a9706f0389b674e7de80c^1...1a463bd
|
Avoids confusion with KS4's searchFields
Fixes the issue of creating an item with a relationship field and trying to shove the entire related object into an ID! field type.
jesstelford
force-pushed
the
improve-relationships
branch
from
1a463bd to
c68dbc6
Compare
jesstelford
force-pushed
the
improve-relationships
branch
from
2095680 to
448e484
Compare
jesstelford
force-pushed
the
improve-relationships
branch
from
448e484 to
51c78e2
Compare
|
@jed I refactored to use your |
|
I'm happy with this PR, who wants to merge it for me? |
|
|
|
@timleslie gave 👍 |
WIP
Addresses requirements in #43