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: extend type CommentProps for new status marker #1856

Closed
wants to merge 4 commits into from

Conversation

Lelith
Copy link
Contributor

@Lelith Lelith commented Jun 20, 2023

Summary

The comments api got extended and delivers now a status field. This field is meant to represent the comment-thread status. Comments can now be marked as resolved, in order to help organize conversations in comments. The currently available states are 'active' and 'resolved'

@Lelith Lelith requested a review from a team as a code owner June 20, 2023 11:35
Copy link

@cniedrich cniedrich left a comment

Choose a reason for hiding this comment

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

LGTM

export type CommentProps = {
sys: CommentSysProps
body: string
status?: CommentStatus
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I think status is always available and defaulting to active

Copy link

@cniedrich cniedrich Jun 20, 2023

Choose a reason for hiding this comment

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

yes! for get, status is always there, just or PUT/POST you do not need to provide it.

see: https://github.com/contentful/comments-api/blob/master/src/api/types.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we would need to update these types for those cases:

export type CreateCommentProps = Omit<CommentProps, 'sys'>
export type UpdateCommentProps = Omit<CommentProps, 'sys'> & {
sys: Pick<CommentSysProps, 'version'>
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@Lelith comments that were created before this property existed will return active

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will change it to non optional :)

@Lelith
Copy link
Contributor Author

Lelith commented Jun 20, 2023

Do we have to mark this release then as breaking / major update?

@Lelith
Copy link
Contributor Author

Lelith commented Jun 20, 2023

closing for now, will update in a different story

@Lelith Lelith closed this Jun 20, 2023
@asaaki asaaki deleted the feat/HOMER-2085_extend_comment_status branch March 12, 2024 19:53
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.

3 participants