-
Notifications
You must be signed in to change notification settings - Fork 153
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
Update proposal for custom attributes schema #456
base: master
Are you sure you want to change the base?
Changes from 3 commits
11be876
72a2238
3205dd0
7a191b5
c12eb3e
c711235
763fd6d
c2d29e2
27243fb
49ed712
74fd87e
b365474
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,13 +8,42 @@ One workaround for "getting all fields" is based on schema introspection, it all | |
|
||
# Proposed solution | ||
|
||
To account for dynamic nature of EAV attributes and the need of "getting all fields" in product search queries, `custom_attributes: [CustomAttribute]!` container will be introduced. | ||
To account for dynamic nature of EAV attributes and the need of "getting all fields" in product search queries, we can introduce `custom_attributes: [CustomAttribute]!` container (recommended approach). | ||
|
||
```graphql | ||
type CustomAttribute { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have to add "type" opinion here to be able to render the appropriate template (datetime, text, multi-select, checkbox ...) E.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Attributes are going to be displayed as information and not as fields you can control, so I don't think it's necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it will be useful to render field properly on UI without complex logic. E.g. for "Date" field - "10-11-2020" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, they are needed for the forms There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have this data already available in the scope of |
||
code: String! | ||
values: [String]! # We want to account fo attributes that have single (text, dropdown) and multiple values (checkbox, multiselect) | ||
} | ||
``` | ||
|
||
We could also make value complex type to be able add more fields in the future, but this doesn't seem necessary at this point. | ||
|
||
```graphql | ||
type CustomAttribute { | ||
code: String! | ||
values: [CustomAttributeValue]! | ||
} | ||
|
||
type CustomAttributeValue { | ||
value: String! | ||
} | ||
``` | ||
|
||
Alternative approach would be is to introduce an interface `custom_attributes: [CustomAttributeInterface]!`, but it seems more complicated. | ||
|
||
```graphql | ||
type CustomAttributeInterface { | ||
code: String! | ||
} | ||
|
||
type SingleValueCustomAttribute extends CustomAttributeInterface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even if this adds more complexity, it's the graphql way to do it and I prefer it |
||
value: String! | ||
} | ||
|
||
type MultipleValuesCustomAttribute extends CustomAttributeInterface { | ||
values: [String]! | ||
} | ||
``` | ||
|
||
Here `value` is JSON-encoded value of the custom attribute. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do fragments bring any value for PWA since we still prefer these flat single structure arrays vs infterfaces?