Skip to content

Add additional actions to CommentThread#162750

Merged
alexr00 merged 20 commits intomicrosoft:mainfrom
marrej:additional-actions-in-comment-thread
Nov 17, 2022
Merged

Add additional actions to CommentThread#162750
alexr00 merged 20 commits intomicrosoft:mainfrom
marrej:additional-actions-in-comment-thread

Conversation

@marrej
Copy link
Contributor

@marrej marrej commented Oct 5, 2022 •
edited
Loading

This PR proposes extension defined additional actions for Comment Threads. (See image below for reference).
Comment

To allow extensions to define additionalActions which are always visible in the CommentThread, a new area has been defined, right under the commentReply. This area contains separator & extension defined actions.
Extensions can defined both single commands and submenus.

Submenus will be rendered as Dropdown buttons, where the first action in the submenu will be treated as the primary action - due to the submenus not carrying Command.
If submenu contains only 1 action, then a single button will be created instead of dropdown button.
In case of no action attached to submenu, the submenu action would be displayed.

NOTE: if there are doubts about AdditionalActions being the correct name for this new area, we should change it.

  • New file has been added to contain the functionality for the Additional Actions
    • Hides the area if there are no active actions or submenu actions
    • sends back only CommentThread to the extension
  • CommentThreadWidget invokes appends this new area in to the CommentThreadBody under the CommentReply
  • CommentFormActions has been modified to provide dropdown buttons along the single buttons (Could be move to a separate file if deemed neccessary, although there would be some unnecessary code duplication)
  • added styling for the new area & elements

partially fixes #156885
fixes #163281

@marrej marrej changed the title # Add additional actions to CommentThread Add additional actions to CommentThread Oct 5, 2022
@marrej marrej changed the title Add additional actions to CommentThread WIP: Add additional actions to CommentThread Oct 5, 2022
@marrej marrej changed the title WIP: Add additional actions to CommentThread Add additional actions to CommentThread Oct 5, 2022
alexr00
alexr00 requested changes Nov 11, 2022
View reviewed changes
Copy link
Member

@alexr00 alexr00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR! This is looking good. I'm seeing some bad alignment of the actions row:
image
image
image

I also don't really like the separator, but I'd like to get @daviddossett's opinion on that.

@marrej
Copy link
Contributor Author

marrej commented Nov 14, 2022 •
edited
Loading

Thank you very much for the review!

Thanks for the PR! This is looking good. I'm seeing some bad alignment of the actions row

I tried to simply fix the problem with the bottom offsetting but it seems to me that there is a bigger problem in play & that is that the height calculation does not fullly align with the actual container height. This problem seems to occur in the actual version with replys as well.

See below that the different amount of replys causes different offsets at the bottom (caused by the height calculation).

image
image
image

So I would suggest we open a bug for that and treat it as a separate issue.

  • Nevertheless I have increase the spacing at the bottom of the AdditionActions so its not mushed against the bottom border.
  • also changed the reply panel padding -> margin so we don't get double offset between the additional actions & the reply area

I also don't really like the separator, but I'd like to get @daviddossett's opinion on that.

The main reasoning for this from my point of view, is the clear separation between the actions connected to the Reply area & the CommentThread. Once the reply area is opened and reply buttons are shown, without the clear visual separation it may be a bit confusing for the users (And may look a bit like a broken view).

image

The main question here is -> do you not like the Idea of the separator? or just the actual styling?

One last question at the end from me

This PR expects that the first button would be shown as primary. Do we want to change it to show all buttons in this row as secondary (reffering to the conversation in the parent issue) ? As there already will be a primary button attached in the Reply area, which would go against the UX guidelines to some extent.
(We could always add some magic group later on, that would enable just some highlight color on a button ,so its differentiable from the others - but I would add this to a separate issue)
What do you think?

@marrej marrej requested a review from alexr00 November 15, 2022 09:58
@alexr00 alexr00 added this to the November 2022 milestone Nov 15, 2022
alexr00
alexr00 requested changes Nov 15, 2022
View reviewed changes
Copy link
Member

@alexr00 alexr00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The main question here is -> do you not like the Idea of the separator? or just the actual styling?

It's the syling I don't like, but we can figure that out outside of this PR.

Do we want to change it to show all buttons in this row as secondary (reffering to the conversation in the parent issue) ?

Yes, let's make all the additional actions buttons secondary.

Thank you for all the other changes, they look good! We'll do a UX review this week and once that (and the secondary button change is done) we should be good to merge this.

@marrej
Copy link
Contributor Author

marrej commented Nov 16, 2022

Sounds good :)
Added the "only secondary buttons" option.

Also noticed that there are some issues with the styling of the dropdown buttons.
image

image

so I added some changes in the button.css & button.ts to take care of the backgroundColor on separator and the whitespace caused by the border-radius
image

Please let me know if you would like to keep this change or whether it should be moved to a different PR, or ignored completely.

@marrej marrej requested a review from alexr00 November 16, 2022 13:53
alexr00
alexr00 reviewed Nov 16, 2022
View reviewed changes
src/vs/base/browser/ui/button/button.css

.monaco-button-dropdown > .monaco-button.monaco-dropdown-button {
border-left-width: 0 !important;
border-radius: 0 2px 2px 0;
Copy link
Member

@alexr00 alexr00 Nov 16, 2022

Choose a reason for hiding this comment

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

@lszomoru, I think with the changes in this file you can get rid of the border-radius in https://github.com/marrej/vscode/blob/f7e26dffdc2f78e8fb75610e5950791016ffcf74/src/vs/workbench/contrib/scm/browser/media/scm.css#L290-L293 and also https://github.com/marrej/vscode/blob/f7e26dffdc2f78e8fb75610e5950791016ffcf74/src/vs/workbench/contrib/scm/browser/media/scm.css#L295-L296

alexr00
alexr00 approved these changes Nov 16, 2022
View reviewed changes
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.

Looks good, thanks for the PR!

@alexr00 alexr00 requested a review from lszomoru November 16, 2022 15:28
@alexr00
Copy link
Member

alexr00 commented Nov 17, 2022

@marrej we reviewed the additional actions UX yesterday and will not be supporting the split button. Please see #163281 (comment) for the whole context.

I'll merge this PR then remove the split button components.

@alexr00 alexr00 merged commit aaac6f9 into microsoft:main Nov 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lszomoru lszomoru lszomoru approved these changes

@alexr00 alexr00 alexr00 approved these changes

Assignees

@alexr00 alexr00

Labels

None yet

Projects

None yet

Milestone

November 2022

Development

Successfully merging this pull request may close these issues.

Additional actions in the Comment Thread Comments: UX exploration for quick reply and other actions

3 participants

@marrej @alexr00 @lszomoru