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

feat(API): Introduce comment replies endpoints #383

Merged
merged 5 commits into from
Aug 24, 2023
Merged

feat(API): Introduce comment replies endpoints #383

merged 5 commits into from
Aug 24, 2023

Conversation

Varpuspaavi
Copy link
Contributor

  • Added comment replies endpoint docs
  • Adjusted comment #create to include locale_ids
  • Adjusted reaction #create to include example

https://github.com/phrase/phrase/pull/11643
https://memsourcegroup.atlassian.net/browse/TSE-869

@Varpuspaavi Varpuspaavi marked this pull request as ready for review August 23, 2023 13:32
Copy link
Collaborator

@jablan jablan left a comment

Choose a reason for hiding this comment

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

Generally OK, just curious about the branch parameter

-H 'Content-Type: application/json'
- lang: CLI v2
source: |-
phrase reactions create \
--project_id <project_id> \
--key_id <key_id> \
--comment_id <comment_id> \
--branch my-feature-branch \
--data '{"branch":"my-feature-branch", "emoji": "👍"]}' \
Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting, does this mean that branch can't be specified with a dedicated switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jablan I'm exactly wondering about this too. In some places we have --branch param and in some just the --data with branch inside 🤔 What do you think? How should we do this so it will definitely work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it would be the best if CLI could have parameters specified through the dedicated switches, and translate that to JSON payload to the API, but not sure if that works and how.

@Varpuspaavi Varpuspaavi changed the title feat(TSE-869): Added documentation for comment replies endpoints feat(TSE-869): Introduce comment replies endpoints Aug 24, 2023
@Varpuspaavi Varpuspaavi changed the title feat(TSE-869): Introduce comment replies endpoints feat(TSE-869) API: Introduce comment replies endpoints Aug 24, 2023
@Varpuspaavi Varpuspaavi changed the title feat(TSE-869) API: Introduce comment replies endpoints feat(TSE-869): Introduce comment replies endpoints Aug 24, 2023
@Varpuspaavi Varpuspaavi changed the title feat(TSE-869): Introduce comment replies endpoints feat(API): Introduce comment replies endpoints Aug 24, 2023
@Varpuspaavi Varpuspaavi merged commit 71351ac into master Aug 24, 2023
7 of 8 checks passed
@Varpuspaavi Varpuspaavi deleted the TSE-869 branch August 24, 2023 10:04
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