Super basic implementation of category page using apollo client for data fetching #52

DrewML · 2018-06-18T21:48:56Z

Note This branch surfaced an issue with the unit/integration test setup in the monorepo. Freezing work on this branch until I've completed #54.

Closes #20

This PR is a:

[ ] New feature
[X] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

Same code from magento-research/venia-pwa-concept#89, now moved over and tests fixed.

Additional information

DrewML · 2018-06-18T21:49:36Z

packages/peregrine/package.json

@@ -13,7 +13,7 @@
    "url": "https://github.com/magento-research/peregrine"
  },
  "scripts": {
-    "build": "babel src -d dist --ignore test.js",
+    "build": "babel src -d dist --ignore '*.test.js,*.spec.js'",


No need to build the test files for distributing. But I also caught this generating built test artifacts, so jest would run all tests twice. Random tech debt from monorepo move

We were using .spec.js for most of initial development, IIRC. When did we start having .test.js files, and why? Should change them.

When did we start having .test.js files

October 31, 2017. We can change it in a separate PR

jimbo · 2018-06-21T17:03:11Z

packages/peregrine/src/List/__tests__/items.spec.js

+    [
+        1,
+        {
+            id: 1,


The tests for List and its child components have been updated as of #62, using almost the same sample data you've got here. You can probably delete these test changes.

Also, I think it's better to have id be a string instead of a number. Is that something we can accomplish with our query?

You can't cast in a GraphQL query, so we'd have to cast on the client anytime that data came in. That would be a PITA, because any query that takes id as an arg would have to be cast back to a number.

jimbo · 2018-06-21T17:03:54Z

packages/venia-concept/src/RootComponents/Category/category.js

-            gallery: PropTypes.string,
-            root: PropTypes.string,
-            title: PropTypes.string
+        id: number,


Likewise, can id be a string?

See #52 (comment)

jimbo · 2018-06-21T17:07:04Z

packages/venia-concept/src/components/Gallery/item.js

+                        currency: string.isRequired
+                    }).isRequired
+                }).isRequired
+            }).isRequired


If an item doesn't have a price, will this full shape be present anyway? Will it just be $0.00 all the way down?

May just be something we have to find out. I'd expect it to always be present, but the schema doesn't have enough info to really tell me.

It's not marked as a non-nullable field in the schema, but I haven't seen that team make much use of non-nullable fields anywhere.

Pinging @eric-bohanon + @paliarush

We are currently not using non-nullable fields in GraphQL schema, but it makes sense to consider this as an improvement. @DrewML feel free to create an issue in magento/graphql-ce project listing any fields that you believe should be made non-nullable.

coveralls · 2018-06-21T17:58:19Z

Pull Request Test Coverage Report for Build 138

3 of 15 (20.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+4.5%) to 67.679%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/venia-concept/src/index.js	0	4	0.0%
packages/venia-concept/src/RootComponents/Category/category.js	0	8	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/venia-concept/src/index.js	1	0.0%

Totals
Change from base Build 137:	4.5%
Covered Lines:	764
Relevant Lines:	1179

💛 - Coveralls

jimbo · 2018-06-21T19:54:07Z

@DrewML Ok, if you resolve conflicts (probably with the List tests) I can approve this.

…ata fetching

DrewML · 2018-06-21T20:27:52Z

@jimbo updated. Luckily conflict was easy

Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions. - Added GraphQL query to `packages/venia-concept/src/RootComponents/Product.js`. - Resolves from URL by using the `url_key` in a GraphQL query. - Can also resolve by SKU. - Modified prop types and render method to accommodate live data shape. - Added `<Currency />` component whose signature matches the Magento GraphQl `Money` type. - Uses the `window.Intl` standard object to format. - Modified the `Gallery` and `ProductImageCarousel` components to use new data shape. - Moved shared constant data URIs to a single `src/shared` folder, to replicate placeholder logic. - Created a shared `propShapes.js` file containing commonly used prop type expressions. - Anticipating that `url_key` would be a common way to navigate, I made a `url_key` utility function. - Added a `makeProductMediaPath` utility function, for turning product image file paths from API responses into relative URLs. - Though [magento/graphql-ce/issues/88](magento/graphql-ce#88) is still a problem for production, I found that **when `magento-sample-data` is installed, it symlinks into the `pub/media` folder so you can use simpler URLs.** - You can see this for yourself with `ls -l <magento-root>/pub/media/catalog/product`. - So I added a `makePathPrepender` function, which we'll later use often, that can create functions like `makeProductMediaPath`. - I hardcoded `/media/catalog/products` in the code, but I also added an environment variable to `.env` and `webpack.config.js` for configuring that URL per instance. - Optimize queries with fragments - Centralize queries in query file to be preprocessed - Make link to product detail on category page - Resolve media URL issue - Test with image galleries Closes #87.

DrewML added the pkg:venia-concept label Jun 18, 2018

DrewML requested review from zetlen and jimbo June 18, 2018 21:48

DrewML commented Jun 18, 2018

View reviewed changes

DrewML force-pushed the apollo-client branch 2 times, most recently from 5f25e3a to 5e005ff Compare June 19, 2018 17:13

jimbo suggested changes Jun 21, 2018

View reviewed changes

DrewML force-pushed the apollo-client branch from 5e005ff to 65ada84 Compare June 21, 2018 17:54

Super basic implementation of category page using apollo client for d…

120f588

…ata fetching

DrewML force-pushed the apollo-client branch from 65ada84 to 120f588 Compare June 21, 2018 20:26

jimbo approved these changes Jun 21, 2018

View reviewed changes

DrewML merged commit 40626e2 into master Jun 21, 2018

DrewML deleted the apollo-client branch June 21, 2018 20:31

zetlen mentioned this pull request Jun 26, 2018

feat(data): Add live GraphQL data to product detail page #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Super basic implementation of category page using apollo client for data fetching #52

Super basic implementation of category page using apollo client for data fetching #52

DrewML commented Jun 18, 2018 •

edited

DrewML Jun 18, 2018

jimbo Jun 18, 2018

DrewML Jun 19, 2018

jimbo Jun 21, 2018

DrewML Jun 21, 2018

jimbo Jun 21, 2018

DrewML Jun 21, 2018

jimbo Jun 21, 2018

DrewML Jun 21, 2018 •

edited

paliarush Jul 13, 2018

coveralls commented Jun 21, 2018 •

edited

jimbo commented Jun 21, 2018

DrewML commented Jun 21, 2018

+                  [
+,
+                      {
+                          id: 1,

Super basic implementation of category page using apollo client for data fetching #52

Super basic implementation of category page using apollo client for data fetching #52

Conversation

DrewML commented Jun 18, 2018 • edited

This PR is a:

Summary

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrewML Jun 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 21, 2018 • edited

Pull Request Test Coverage Report for Build 138

💛 - Coveralls

jimbo commented Jun 21, 2018

DrewML commented Jun 21, 2018

DrewML commented Jun 18, 2018 •

edited

DrewML Jun 21, 2018 •

edited

coveralls commented Jun 21, 2018 •

edited