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

fetch data for PDP #75

Merged
merged 21 commits into from
May 17, 2018
Merged

fetch data for PDP #75

merged 21 commits into from
May 17, 2018

Conversation

jshimko
Copy link
Contributor

@jshimko jshimko commented May 1, 2018

This PR relies on reactioncommerce/reaction#4218 to work (which should hopefully be merged soon)

Updates:

  • fetch product item data using URL slug or ID
  • update any PDP components with mapped data
  • update mock data to match GraphQL data

* master:
  fix: helmet name undefined
* master: (21 commits)
  update jsdoc
  grammer
  add adding observable docs
  jsdocs
  Update test snapshots
  Add inventory status badges to variants/options lists.
  move comments into jsdoc in each file
  add links to files
  add definition location
  Add Badge component test
  add UIStore docs
  add authstore docs
  formatting
  update docs link
  add mobx documentation
  Add theme provider to ProductGrid
  Add sale and bestseller badge types.
  Fix SSR caching issue
  Upgrade apollo and MUI dependencies.
  Refactor inventory status determination to utility functions.
  ...
@spencern
Copy link
Contributor

spencern commented May 5, 2018

@jshimko How close are we to having this PR be ready for review?

@jshimko jshimko requested a review from nnnnat May 16, 2018 22:06
@jshimko
Copy link
Contributor Author

jshimko commented May 16, 2018

This is still depending on the catalog schema PR in the main Reaction repo (reactioncommerce/reaction#4218), but it's ready to be tested/reviewed.

reaction init -b refactor-4196-aldeed-catalog-schema-changes catalog-schema
cd catalog-schema
docker-compose up -d --build

Then start up this PR branch and everything should work. You should see the PDP is now completely populated by the GraphQL API data.

* master: (38 commits)
  Fix eslint error
  Correct typo
  Fix typos
  fix: remove extra bracket
  docs: fix method name
  test: fix typo
  feat: enable paganation for tag page product grid
  fix: add proptype definition for pageInfo
  refactor: shallow render route
  refactor: fix typos
  refactor: make prop required
  test: update snapshot
  test: add page info to test
  refactor: increase page limit to match reaction default
  refactor: add pageInfo prop and guard
  refactor: make pageInfo required
  test: add PageStepper test snapshots
  test: add PageStepper test
  refactor: remove with theme
  refactor: put space between previous and next buttons
  ...

# Conflicts:
#	src/containers/catalog/catalogItems.gql
#	src/containers/catalog/withCatalogItems.js
if (graphQLErrors) {
graphQLErrors.forEach(({ message, locations, path }) => {
// eslint-disable-next-line no-console
console.log(`[GraphQL error]: Message: ${message}, Location: ${JSON.stringify(locations)}, Path: ${JSON.stringify(path)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, maybe... but that logger is using chalk (for terminal colors) and these particular errors can happen on the server and the client. I think we probably need a more isomorphic solution for this use case. We could certainly use Bunyan for that. Then we'd have all of the same API's we're used to in Reaction (for piping logs to other places, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I wasn't sure if our lib/logger would work in these cases. I'm good with just doing the console.error here for now and maybe we revisit our logger soon in another PR

Copy link
Contributor

@nnnnat nnnnat left a comment

Choose a reason for hiding this comment

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

Overall this is looking good, had no problem getting it to work with the new catalog updates in RC.

I have a question about some logging and once that's resolved I'll merge.

Great work @jshimko

@jshimko
Copy link
Contributor Author

jshimko commented May 17, 2018

@nnnnat FYI, it's hidden in the Github UI now because I pushed changes, but I did respond to the logging questions.

#75 (comment)

@nnnnat
Copy link
Contributor

nnnnat commented May 17, 2018

@spencern @aldeed I think this is good 2 go, but should we wait on PR 4218 before we merge this one?

@nnnnat nnnnat merged commit e28a07f into master May 17, 2018
@nnnnat nnnnat deleted the feat-17-pdp-data-fetching branch May 17, 2018 18:18
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.

None yet

3 participants