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

api: Discourse API "proxy" endpoint at /api/comments #819

Merged
merged 11 commits into from
Dec 4, 2019
Merged

Conversation

iAdramelk
Copy link
Contributor

@iAdramelk iAdramelk commented Nov 25, 2019

API handle to return comments count to use in blog.dvc.org (see iterative/blog#8).

@shcheklein shcheklein temporarily deployed to dvc-org-pr-819 November 25, 2019 10:12 Inactive
server.js Show resolved Hide resolved
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

a few minor questions

@jorgeorpinel

This comment has been minimized.

pages/api/comments.js Outdated Show resolved Hide resolved
server.js Outdated Show resolved Hide resolved
@iAdramelk

This comment has been minimized.

@shcheklein

This comment has been minimized.

@shcheklein shcheklein temporarily deployed to dvc-org-pr-819 November 26, 2019 02:35 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-pr-819 November 26, 2019 02:37 Inactive
@jorgeorpinel jorgeorpinel changed the title Forum proxy Discourse API "proxy" endpoint at /api/comments Nov 26, 2019
@iterative iterative deleted a comment from iAdramelk Nov 26, 2019
@jorgeorpinel jorgeorpinel changed the title Discourse API "proxy" endpoint at /api/comments api: Discourse API "proxy" endpoint at /api/comments Nov 26, 2019
@jorgeorpinel jorgeorpinel added website: eng-doc DEPRECATED JS engine for /doc A: docs Area: user documentation (gatsby-theme-iterative) labels Nov 26, 2019
src/consts.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Some notes on the response messages:

pages/api/comments.js Outdated Show resolved Hide resolved
pages/api/comments.js Outdated Show resolved Hide resolved
pages/api/comments.js Outdated Show resolved Hide resolved
@iAdramelk
Copy link
Contributor Author

Made some changes in error responses

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Thanks Alex! Just one more suggestion in #819 (comment) and it seems you may need to solve conflicts with master branch.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Dec 2, 2019

I guess we could also add a small section in REAMDE describing this endpoint's spec? Just so in the future we remember why it exists, and what request/response (payloads) are expected.

@iAdramelk
Copy link
Contributor Author

@jorgeorpinel

Added WEBSITE_HOST and comment in the begining of the pages/api/comments.js. I think it's better place for it then readme.md – readme is about dvc.org site, and this endpoint is not used by site, but is used by separate blog.

@shcheklein
Copy link
Member

@iAdramelk please resolve the conflicts ^^

@iAdramelk
Copy link
Contributor Author

@shcheklein Rebased it.

@shcheklein shcheklein merged commit cc97843 into master Dec 4, 2019
@shcheklein
Copy link
Member

thanks @iAdramelk !

Comment on lines +1 to +10
/*
* This API endpoint is used by https://blog.dvc.org
* to get comments count for the post, it gets
* discuss.dvc.org topic url as a param and returns
* comments count or error.
*
* It made this way to configure CORS, reduce user's payload
* and to add potential ability to cache comments count
* in the future.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll rewrap this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: docs Area: user documentation (gatsby-theme-iterative) website: eng-doc DEPRECATED JS engine for /doc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants