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

Allow individual comments to be marked as draft #173305

Merged
merged 6 commits into from
Mar 28, 2023

Conversation

hermannloose
Copy link
Contributor

@hermannloose hermannloose commented Feb 3, 2023

This is a proposal for #171166.

The names are rough suggestions, I am not sure if "visibility" is more descriptive than e.g. CommentDraftState.

CommentThread.hasDraftComments is included to show how we would conceptually want this to roll up into the thread. The surrounding features suggested in #171166 all operate on threads, but hasDraftComments does not necessarily need to become part of the API surface, it could be implicit and the relevant parts like line gutter decorations could just do the same iteration ad-hoc.

Staying with the example of line gutter decorations for illustration, our ideal UX would be:

  • resolved thread: $(comment)
  • unresolved thread: $(comment-unresolved)
  • thread with a draft, regardless of status: $(comment-draft)

Note that in this particular case for the UI, it ends up looking like the "draft" state is mutually exclusive with resolved or unresolved CommentThreadState, but they are separate things, e.g. there could be two icons for lines that have a thread (or threads) with drafts, one icon to give the state, one icon to indicate presence of drafts.

@hermannloose
Copy link
Contributor Author

hermannloose commented Feb 3, 2023

cc @JonasMa @laurentlb @gkatja

@alexr00 alexr00 added this to the February 2023 milestone Feb 3, 2023
@alexr00 alexr00 self-assigned this Feb 3, 2023
@hermannloose hermannloose force-pushed the comment-drafts-proposal branch from 20985e9 to e63a47d Compare February 3, 2023 15:14
@hermannloose hermannloose force-pushed the comment-drafts-proposal branch from e63a47d to 51396e6 Compare February 13, 2023 13:47
@hermannloose
Copy link
Contributor Author

Any initial thoughts on the proposal, regarding naming or structure?

@alexr00
Copy link
Member

alexr00 commented Feb 16, 2023

@hermannloose I haven't had a chance to give this the review it deserves. I will plan to review it during our next iteration (starts in 1.5 weeks).

@hermannloose hermannloose force-pushed the comment-drafts-proposal branch from 51396e6 to cc614c8 Compare March 6, 2023 09:28
@hermannloose
Copy link
Contributor Author

I dropped hasDraftComments as per #171166 (comment).

visibility vs state did not sound as decided yet, so I kept it in for now. I think visibility is more descriptive than state here, so personally I would still lean towards visibility—also because it avoids potential API user confusion around CommentState just being CommentThreadState but at the Comment level; as mentioned above, they are separate and I would prefer the more obvious difference in name.

No strong opinions on Pending vs Draft but that also sounded like it was still up for debate, so I left it as is for now.

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

With the visibility to state change I would accept a complete PR to add the published/draft feature.

@alexr00 alexr00 modified the milestones: March 2023, April 2023 Mar 22, 2023
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR and for your patience with my slow review!

@alexr00 alexr00 merged commit de3b0db into microsoft:main Mar 28, 2023
@hermannloose hermannloose deleted the comment-drafts-proposal branch March 29, 2023 12:45
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants