Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Storefront properties for Product Attributes #490
base: master
Are you sure you want to change the base?
Add Storefront properties for Product Attributes #490
Changes from 2 commits
f873651
0294d2c
a9e590e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is good but please change the location of your file to this file
design-documents/graph-ql/coverage/custom-attributes/attributes-metadata.graphqls
https://github.com/magento/architecture/pull/456/files#diff-113bf18a368d3d5c2ee6e9956737276d3f7fcc4306fe477c8dc67f937d0b8839R1
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.
Perhaps name these
use_in_layered_navigation
,use_in_product_listing
, anduse_in_catalog
for consistency?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.
I understand these names come directly from the backend.
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.
Yes, the idea was to reflect the same configuration names as in the backend. So there will be consistency in dev docs too. @keharper can help with the naming.
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.
I'm in favor of changing
used_in_product_listing
touse_in_product_listing
for consistency, butuse_in_catalog
is too dissimilar from "Visible on Catalog Pages on Storefront" to easily make the connection. I recommend changingvisible_on_catalog_storefront
tovisible_on_catalog_pages
.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.
Okay. I will update it. Thanks!
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.
I don't think this needs to be included. The way I understand it, this is more for y'all or the search team. If
Use in Search
is set toYES
, then users can search by the product attribute values (ex: "Green"), and matching products will be included in the result set.Venia won't do anything with this setting.