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

Add Storefront properties for Product Attributes #490

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

Conversation

prabhuram93
Copy link
Contributor

Problem

PWA-1665
Need to control the use of product attributes on PWA Venia Storefront

Solution

Added a field storefront_properties in the customAttributeMetadata query to expose admin configuration on product attributes related to storefront.

Requested Reviewers

# See COPYING.txt for license details.

type Query {
customAttributeMetadata(attributes: [AttributeInput!]!): CustomAttributeMetadata @resolver(class: "Magento\\EavGraphQl\\Model\\Resolver\\CustomAttributeMetadata") @doc(description: "The customAttributeMetadata query returns the attribute type, given an attribute code and entity type") @cache(cacheable: false)
Copy link
Contributor

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

Comment on lines 22 to 25
used_in_product_listing: Boolean
position: Int
visible_on_catalog_storefront: Boolean
use_in_layered_navigation: UseInLayeredNavigationOptions

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, and use_in_catalog for consistency?

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.

Copy link
Contributor Author

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.

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 to use_in_product_listing for consistency, but use_in_catalog is too dissimilar from "Visible on Catalog Pages on Storefront" to easily make the connection. I recommend changing visible_on_catalog_storefront to visible_on_catalog_pages.

Copy link
Contributor Author

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!

@supernova-at
Copy link

supernova-at commented May 7, 2021

Layered Navigation

Looking at page https://develop.pwa-venia.com/default/venia-dresses.html?page=1, a category page on Venia.
The following queries are issued that relate to layered navigation / filters:

  1. getProductFiltersByCategory
  2. getFilterInputsForCategory

Am I correct that the proposal is that we would take the attribute_code, issue a customAttributesMetadata query, and then pull the storefront_properties off of that?

If so, it looks like the customAttributesMetadata query needs more than just the attribute_code, it also wants an entity_type. Acceptable values are catalog_product, catalog_category, or customer. For this query I'm assuming catalog_category.

@supernova-at
Copy link

supernova-at commented May 7, 2021

Product Detail Page

Here's the product details query we send. Here's the fragment.

Looks like we could pull this attribute_code and issue the customAttributesMetadata query with entity_type = catalog_product? We'd use use_in_product_listing and position from there on the PDP.

@supernova-at
Copy link

Category Page

Here's the category query and fragment.

Looks like we'd need to add the attribute_code field and do the additional customAttributesMetadata query with entity_type = catalog_category.

@supernova-at
Copy link

Sorting

We get all the aggregations in the getProductFiltersByCategory query, but we should not rely on the order of the attributes returned from GraphQL for rendering.

It would be ideal to have the ProductInterface expose a field called position (I think you are proposing that but I don't see it here in this PR). Without that, we'd have to send the additional customAttributeMetadata query and pull the position off of storefront_properties. Sort by position and render.

@supernova-at
Copy link

Search

Use in Search

This setting would remain a known bug / unsupported. Attributes are always used in search.

Use in Search Results Layered Navigation

Here's the query for filters for the search page. We could send the attribute_code to customAttributesMetadata to get the storefront_properties but I don't see anything in there for layered navigation specific to search.

Will this remain unsupported as well?

@prabhuram93
Copy link
Contributor Author

catalog_product

For product attributes it should be catalog_product

@prabhuram93
Copy link
Contributor Author

Sorting

We get all the aggregations in the getProductFiltersByCategory query, but we should not rely on the order of the attributes returned from GraphQL for rendering.

It would be ideal to have the ProductInterface expose a field called position (I think you are proposing that but I don't see it here in this PR). Without that, we'd have to send the additional customAttributeMetadata query and pull the position off of storefront_properties. Sort by position and render.

I will see the feasibility of adding this in the aggregations itself without compromising the backend performance. I should be able to get the answer soon.

@prabhuram93
Copy link
Contributor Author

Search

Use in Search

This setting would remain a known bug / unsupported. Attributes are always used in search.

Use in Search Results Layered Navigation

Here's the query for filters for the search page. We could send the attribute_code to customAttributesMetadata to get the storefront_properties but I don't see anything in there for layered navigation specific to search.

Will this remain unsupported as well?

we can support both these configurations. Will update the PR.

@@ -23,11 +23,13 @@ type StorefrontProperties {
position: Int
visible_on_catalog_storefront: Boolean
use_in_layered_navigation: UseInLayeredNavigationOptions
use_in_search_results_layered_navigation: Boolean
use_in_search: Boolean
Copy link

@supernova-at supernova-at May 11, 2021

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 to YES, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants