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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Update proposal for custom attributes schema #456

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
11be876
Update custom attributes container proposal to account for attributes…
melnikovi Oct 20, 2020
72a2238
Update custom attributes container proposal to account for attributes…
melnikovi Oct 20, 2020
3205dd0
Update custom attributes container proposal to account for attributes…
melnikovi Oct 20, 2020
7a191b5
modifying proposal
cpartica Feb 5, 2021
c12eb3e
adding interfaces on inputs
cpartica Feb 10, 2021
c711235
refactor some things on product attributes
cpartica Feb 10, 2021
763fd6d
- refactor options
cpartica Feb 10, 2021
c2d29e2
- reactor types
cpartica Mar 5, 2021
27243fb
- refactor container explanation
cpartica Mar 5, 2021
49ed712
- rename option ids
cpartica Mar 5, 2021
74fd87e
refactoring to simplify query and schema
cpartica Mar 13, 2021
b365474
refactoring to simplify query and schema
cpartica Mar 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion ...gn-documents/graph-ql/coverage/custom-attributes/custom-attributes-container.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

```graphql
type CustomAttribute {
Copy link
Member

@mslabko mslabko Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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.

"attributes":[
    {
      "code":"multiselect_attribute",
      "type":"multiselect",
      "values":[
            "Option 1",
            "Option 2"
      ]
    }
  ]

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@mslabko mslabko Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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"

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, they are needed for the forms

Copy link
Contributor

Choose a reason for hiding this comment

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

We have this data already available in the scope of customAttributeMetadata query. Since update frequency for attribute metadata and product attribute values is different, it makes sense to separate them to avoid excessive payload size.

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]!`.

```graphql
type CustomAttributeInterface {
code: String!
}

type SingleValueCustomAttribute extends CustomAttributeInterface {
Copy link
Contributor

@cpartica cpartica Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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.
Expand Down