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

Compatibility with Magento 2.3.1 #966

Closed
wants to merge 5 commits into from

Conversation

tjwiebell
Copy link
Contributor

@tjwiebell tjwiebell commented Feb 27, 2019

Description

Make changes to Venia that supports the backwards incompatible changes to GraphQL being release with Magento 2.3.1.

Related Issue

Closes #842

Motivation and Context

Venia must be compatible with 2.3.1 release

How Has This Been Tested?

Manually tested locally and ran jest

Screenshots (if appropriate):

Proposed Labels for Change Type/Package

Checklist:

  • I have read the CONTRIBUTING document.
  • I have linked an issue to this PR.
  • I have indicated the change type and relevant package(s).
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All CI checks are green (linting, build/deploy, etc).
  • At least one core contributor has approved this PR.

lewisvoncken and others added 2 commits February 27, 2019 11:34
…lving errors and added Category Thumbnail Label Support

venia-concept/src/queries/getCategoryList.graphql
  15:21  error  Field "small_image" of type "ProductImage" must have a selection of subfields. Did you mean "small_image { ... }"?  graphql/template-strings

packages/venia-concept/src/queries/getProductDetail.graphql
  14:13  error  Field "description" of type "ComplexTextValue" must have a selection of subfields. Did you mean "description { ... }"?  graphql/template-strings

packages/venia-concept/src/RootComponents/Category/category.js
  23:21  error  Field "small_image" of type "ProductImage" must have a selection of subfields. Did you mean "small_image { ... }"?  graphql/template-strings

packages/venia-concept/src/RootComponents/Product/Product.js
  25:17  error  Field "description" of type "ComplexTextValue" must have a selection of subfields. Did you mean "description { ... }"?  graphql/template-strings
- Apply schema changes to additional places not covered in original PR
- Update tests to reflect changes to expected shapes/shema
- Fix bug in media URL generator that didn't support http protocol
@vercel
Copy link

vercel bot commented Feb 27, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

- Remove unused method related to image labels that makes behavior inconsistent
Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

@tjwiebell Nice work! Thanks for taking this one over. Prop types will click eventually. 😅

small_image: string
small_image: shape({
url: string.isRequired
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
})
}).isRequired

Because you're accessing small_image.url, render() will throw if it's not an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

small_image: string.isRequired,
small_image: shape({
url: string.isRequired
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

In this component, the prop type for data should be simply array. The current arrayOf(shape()) is unnecessary because Gallery does nothing with the data prop but send it along to GalleryItems; even if data were null, this component would not throw during render, even if one of its children might.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this suggested change

small_image: string.isRequired,
small_image: shape({
url: string.isRequired
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}),
}).isRequired,

Because you're accessing small_image.url, renderImage() will throw if it's not an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

small_image: string.isRequired,
small_image: shape({
url: string.isRequired
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

The name, price, and small_image prop types are unnecessary in this component. GalleryItems needs an array of objects that each have an id property in order to render successfully, but no more than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unnecessary props

description: string
description: shape({
html: string.isRequired
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
})
}).isRequired

Because you're accessing description.html, render() will throw if it's not an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

url_key: string.isRequired,
small_image: shape({
url: string.isRequired
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}),
}).isRequired,

Because you're accessing small_image.url, render() will throw if it's not an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

Works great!

@tjwiebell
Copy link
Contributor Author

Closing in favor of #990 with cherry-picked commits to merge into release/2.0.

@tjwiebell tjwiebell closed this Mar 4, 2019
@tjwiebell tjwiebell deleted the tommy/2.3.1-compat-rebased branch March 20, 2019 17:07
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