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

Nested Fields [WiP] #3300

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
4 participants
@wmertens
Copy link
Contributor

commented Aug 11, 2016

@JedWatson for your consideration :)

Note that it removes use of legacy endpoints.

@wmertens wmertens force-pushed the Yaska:nested branch 3 times, most recently from da3e42d to 697bf52 Aug 11, 2016

@JedWatson

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

@wmertens can you open this PR against the nested-fields branch instead of master please?

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

I can't, it's a rebase and squash of the nested-fields branch… I can do a force-push on the nested-fields branch.

@wmertens wmertens force-pushed the Yaska:nested branch 2 times, most recently from 14e0783 to b845a1f Aug 12, 2016

@@ -56,15 +58,15 @@ module.exports = Field.create({
getFilename () {
return this.state.userSelectedFile
? this.state.userSelectedFile.name
: this.props.value.filename;
: this.props.value && this.props.value.filename;

This comment has been minimized.

Copy link
@JedWatson

JedWatson Aug 12, 2016

Member

I think we're solving this the wrong way, props.value should always be an object, and we should fix that somewhere higher than this to reduce code branches.

The probably correct but more-work way would be to dispatch new item values / nested item values from something central that can get the default value from each field type (which would take into account field defaults)

Quicker way is putting value in state and updating it with defaults correctly applied in componentWillUpdate, maybe?

@@ -101,7 +103,7 @@ list.prototype.addToSchema = function () {
);
}
fields[path] = createField(path, fieldsSpec[path]);
fieldsArray.push(path);
paths.push(path);

This comment has been minimized.

Copy link
@JedWatson

JedWatson Aug 12, 2016

Member

paths means something specific on Field Types - it's a nested set of strings you can pull values from when processing incoming data (or use to generate the names of form fields)

neither of those will work in this case, which is why I mirrored the fieldsArray concept from the main List class

This comment has been minimized.

Copy link
@wmertens

wmertens Aug 12, 2016

Author Contributor

Oh - I renamed it because fieldArray was actually an array of names and not the actual fields like in the List class.

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2016

@JedWatson this last commit adds the possibility to replace NumbersArray and LocalFiles with simply {type: Types.List, field: {type: Types.Number/Types.File, ...}}. It just won't handle the update correctly at all right now.

@wmertens wmertens force-pushed the Yaska:nested branch from ae9b3c0 to c6a6028 Aug 12, 2016

@webteckie

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2016

@wmertens in terms of terminology, List may be an overloaded term in keystone if we also have a list type and may get confusing. Perhaps use Collection or something that's less potentially confusing?

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2016

We could name it Array, like the other types similar to built-in types?

On Sat, Aug 13, 2016 at 1:36 AM Carlos Colon notifications@github.com
wrote:

@wmertens https://github.com/wmertens in terms of terminology, List may
be an overloaded term in keystone if we also have a list type and may get
confusing. Perhaps use Collection or something that's less potentially
confusing?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3300 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADWlo0Zqc8O7IrDWZ1d6N6Ir79PzZFDks5qfQODgaJpZM4Jh1Jd
.

@wmertens wmertens referenced this pull request Aug 13, 2016

Closed

Sub-collections #153

@webteckie

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2016

Ideally what's now a List [model] would have probably made more sense to be a Collection [model] and then List as a type would have made a lot of sense, IMO. Is it really an Array in the true sense of the word? Another possibility is to just call it Nested? Or Composite? Or ...

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2016

This type is truly an array of mongoose subdocs, and I would like it to
also be an array of singular types so it can replace LocalFiles without
change.

Later the schemabuilder can replace a literal embedded array with an
instance of this type.

On Sat, Aug 13, 2016, 2:04 PM Carlos Colon notifications@github.com wrote:

Ideally what's now a List [model] would have probably made more sense to
be a Collection [model] and then List as a type would have made a lot of
sense, IMO. Is it really an Array in the true sense of the word? Another
possibility is to just call it Nested? Or Composite? Or ...


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3300 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADWltCdrX6FwFOenDGql0Wb8HcaKAMlks5qfbLmgaJpZM4Jh1Jd
.

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2016

Something to keep in mind is that in keystone there is the data as
presented in the form post, the data as held in the server memory by
mongoose, and the data in mongodb.

Then there is the form schema and the mongoose schema.

All are alike but not the same.

On Sat, Aug 13, 2016, 4:39 PM Wout Mertens wout.mertens@gmail.com wrote:

This type is truly an array of mongoose subdocs, and I would like it to
also be an array of singular types so it can replace LocalFiles without
change.

Later the schemabuilder can replace a literal embedded array with an
instance of this type.

On Sat, Aug 13, 2016, 2:04 PM Carlos Colon notifications@github.com
wrote:

Ideally what's now a List [model] would have probably made more sense to
be a Collection [model] and then List as a type would have made a lot of
sense, IMO. Is it really an Array in the true sense of the word? Another
possibility is to just call it Nested? Or Composite? Or ...


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3300 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADWltCdrX6FwFOenDGql0Wb8HcaKAMlks5qfbLmgaJpZM4Jh1Jd
.

@christroutner

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2016

Something to keep in mind is that in keystone there is the data as
presented in the form post, the data as held in the server memory by
mongoose, and the data in mongodb.

Then there is the form schema and the mongoose schema.

All are alike but not the same.

This would be really good concept to expand on in the documentation.

@JedWatson

This comment has been minimized.

Copy link
Member

commented Aug 14, 2016

@webteckie I've avoided calling List collection because it has meaning in MongoDB. We're hoping, maybe even before the end of the year, to implement an abstraction layer that lets Keystone Lists work with RethinkDB / Postgres / etc. at which point the List will represent a mapping to a Table instead of a Collection, an Item maps to a Record instead of a Document, etc.

I named the field type List because it will, in most ways, behave like a top-level List; it has an array of fields, can validate and process incoming data, contains (in MongoDB) documents, etc.

Array works for the current simple types (TextArray, etc) because those truly are a simple array storing the specified simple data type.

I'm not sure we should replace the ...Array fields just yet; if they're useful (sometimes they are) they allow us to do certain nice things in the UI that would be much more complicated to implement with a nested schema (e.g. format TextArray values into a comma-delimited list for display in the grid view), but I'm open to discussing that. It's certainly nice that we could based on this work.

@JedWatson

This comment has been minimized.

Copy link
Member

commented Aug 14, 2016

@wmertens unless I'm mistaken, you've made it so that Types.List can take a single field instead of a Fields array... seems like that would introduce a lot of internal complexity, would we be better having a Types.Array field that accepts a field option? The UI design between each behaviour is quite different (i.e. nested lists have a label for each field, compared to the UI for Types.TextArray)

I'm leaning towards tackling adding that as a separate block of work once the main List type has landed and we have proper nested lists support..?

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2016

This is why I think we should have an "Object" Field so the array field
only takes a single field and the Object field is responsible for showing
labels

On Sun, Aug 14, 2016, 11:33 AM Jed Watson notifications@github.com wrote:

@wmertens https://github.com/wmertens unless I'm mistaken, you've made
it so that Types.List can take a single field instead of a Fields
array... seems like that would introduce a lot of internal complexity,
would we be better having a Types.Array field that accepts a field
option? The UI design between each behaviour is quite different (i.e.
nested lists have a label for each field, compared to the UI for
Types.TextArray)

I'm leaning towards tackling adding that as a separate block of work once
the main List type has landed and we have proper nested lists support..?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3300 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADWlgMlaY99EMqDr-fQAH8ZNfGkeorMks5qfuDkgaJpZM4Jh1Jd
.

@wmertens wmertens force-pushed the Yaska:nested branch from c105eb1 to df0b687 Aug 16, 2016

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

@JedWatson:

  • rebased
  • removed single field thing for now
  • make fieldsArray an actual array of fields
  • added bonus commit for thumbnails of files. It's not really fit for merge since it assumes flexbox and uses inline styles but wondering what you think

@wmertens wmertens force-pushed the Yaska:nested branch 2 times, most recently from 8df77d9 to 53e9e9c Aug 16, 2016

@wmertens wmertens force-pushed the Yaska:nested branch 2 times, most recently from caeca37 to 104b01d Aug 24, 2016

JedWatson and others added some commits Aug 4, 2016

@wmertens wmertens force-pushed the Yaska:nested branch from 104b01d to f9ac1ce Aug 24, 2016

@wmertens

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2016

Closing this, apart from the squashing of Jed's commits this is the same as #3282 plus #3403

@wmertens wmertens closed this Aug 26, 2016

@wmertens wmertens deleted the Yaska:nested branch Aug 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.