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

BREAKING CHANGE: Include collection pages in the GET pages endpoint #31

Merged
merged 3 commits into from
Nov 20, 2019

Conversation

taufiq
Copy link
Contributor

@taufiq taufiq commented Nov 20, 2019

This PR now shows all collection pages, along with the existing simple pages on the GET pages endpoint.

Context

Initially only simple pages were shown in the GET pages endpoint. The team decided that collection pages would be included as well as we were scraping the 'Collections' tab in the frontend.

Implications

This pull request needs to be merged into master along with the frontend PR or else the frontend will break.

THIS IS FOR THE FRONTEND BRANCH `feat/flatten-collection-into-pages`.

This commit will include collection pages and tokenize the items shown in the `GET pages` endpoint
into types `simple-page` and `collection`.
This commit adds in comments to explain how the logic in `GET pages`
endpoint works.
routes/pages.js Outdated
const CollectionPage = new File(access_token, siteName)
const collectionPageType = new CollectionPageType(collectionName)
CollectionPage.setFileType(collectionPageType)
const collectionPages = await Bluebird.map(await CollectionPage.list(), (item) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to use await Bluebird.map here. A simple map over the result of await CollectionPage.list() works because there is no async call within the map.

Copy link
Contributor

Choose a reason for hiding this comment

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

The PR looks good otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh yeah good point alright will fix that

There was no need to call `Bluebird.map` with a promise that has been
resolved (`await CollectionPage.list()`)
Copy link
Contributor

@prestonlimlianjie prestonlimlianjie left a comment

Choose a reason for hiding this comment

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

Lgtm

@taufiq taufiq merged commit c4e7bd9 into master Nov 20, 2019
@prestonlimlianjie prestonlimlianjie deleted the feat/flatten-collection-into-pages branch November 21, 2019 02:17
harishv7 pushed a commit that referenced this pull request Feb 17, 2023
…#31)

* feat!: include collection pages in pages endpoint

THIS IS FOR THE FRONTEND BRANCH `feat/flatten-collection-into-pages`.

This commit will include collection pages and tokenize the items shown in the `GET pages` endpoint
into types `simple-page` and `collection`.

* style: add comments on `GET pages` endpoint

This commit adds in comments to explain how the logic in `GET pages`
endpoint works.

* refactor(pages): remove redundant Bluebird map

There was no need to call `Bluebird.map` with a promise that has been
resolved (`await CollectionPage.list()`)
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.

2 participants