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.

Sign up for GitHub

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

Jump to bottom

Create new topic for the Peregrine REST API client #259

Merged
merged 4 commits into from
Sep 18, 2018
Merged

Create new topic for the Peregrine REST API client #259

merged 4 commits into from
Sep 18, 2018

Conversation

jcalcaben
Copy link
Contributor

This PR is a:

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

Summary

When this pull request is merged, it will create a new documentation page for the Peregrine REST API client.

@jcalcaben jcalcaben added this to the Sprint 24 milestone Aug 30, 2018
@jcalcaben jcalcaben self-assigned this Aug 30, 2018
@jcalcaben jcalcaben requested a review from zetlen August 30, 2018 14:31
@ericerway ericerway modified the milestones: Sprint 24, Sprint 25 Sep 10, 2018
@ericerway
Copy link

Moving review and merge tasks to Sprint 25. Thanks!

zetlen
zetlen suggested changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Looks great and well edited as always. Just one structural change and a couple of renames.

packages/pwa-devdocs/src/_data/peregrine.yml Outdated
@@ -18,5 +18,8 @@ entries:
- label: List
url: /peregrine/reference/list/

- label: REST API client
url: /peregrine/reference/rest-api-client/

Copy link
Contributor

Choose a reason for hiding this comment

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

In Peregrine, a Component isn't a generic term like it is for other contexts. Peregrine includes utilities and stuff, but its Components are literally React Components. So I don't think we should list the REST API client in Components, since it's just a JavaScript module and not a React component.

I think this might demand a separate section for "Modules", with short explanations that Components are React Components and Modules are other utilities.

packages/pwa-devdocs/src/peregrine/reference/rest-api-client/index.md Outdated

### Error handling

Promoises returned by `request()` are rejected if the server responds with an HTTP error code within the 4xx-5xx range.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, Promoises.

packages/pwa-devdocs/src/peregrine/reference/rest-api-client/index.md Outdated

To use a rolling request, set the `cache` configuration option to `reload` or `no-store`.

### The `Magento2ApiRequest` class
Copy link
Contributor

Choose a reason for hiding this comment

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

This inconsistency is from me, not you, but in retrospect, we're going back and forth with calling this class Magento2ApiRequest and M2ApiRequest. In the code, the property on the request function is named M2ApiRequest, so we should replace all instances of Magento2ApiRequest with M2ApiRequest.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 930

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 35 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-1.2%) to 30.46%
Files with Coverage Reduction New Missed Lines %
packages/pwa-buildpack/src/WebpackTools/plugins/MagentoRootComponentsPlugin/index.js 1 88.89%
packages/venia-concept/src/components/Header/header.js 3 0.0%
packages/venia-concept/src/RootComponents/Category/category.js 4 0.0%
packages/venia-concept/src/components/CategoryList/categoryList.js 9 0.0%
packages/venia-concept/src/components/Gallery/item.js 18 0.0%
Totals Coverage Status
Change from base Build 914: -1.2%
Covered Lines: 519
Relevant Lines: 1709
💛 - Coveralls

zetlen
zetlen approved these changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you!

@zetlen zetlen merged commit 23826a8 into master Sep 18, 2018
@zetlen zetlen deleted the jimothy/rest-client-doc branch September 18, 2018 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zetlen zetlen zetlen approved these changes

Assignees

@jcalcaben jcalcaben

Labels
pkg:pwa-devdocs
Projects
None yet
Milestone
Sprint 25
Development

Successfully merging this pull request may close these issues.
4 participants
@jcalcaben @ericerway @coveralls @zetlen