Skip to content

Fix comment layout & use bin as delete icon#4285

Merged
alexr00 merged 8 commits intomicrosoft:mainfrom
Thomas1664:comment-layout-fixes
Dec 19, 2022
Merged

Fix comment layout & use bin as delete icon#4285
alexr00 merged 8 commits intomicrosoft:mainfrom
Thomas1664:comment-layout-fixes

Conversation

@Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Dec 3, 2022

Continuation of #4057

The current delete icon is a cross (x) but I think that we associate a cross more with close. Therefore, I think the bin icon expresses 'delete' better than a cross.

I also changed the comment action bar to hiding instead of removing itself if the comment is not hovered. Furthermore, the action bar used to not show on focus. I fixed this bug as well.

I want to align the secondary button on comments to the right as well because the user needs less eye movement to see both buttons.

46rouser
46rouser previously approved these changes Dec 5, 2022
@alexr00 alexr00 added this to the December 2022 milestone Dec 5, 2022
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.

Thanks for the PR! I'm seeing an x overlayed over the bin icon:

image

Other than that, it looks good.

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!

@alexr00 alexr00 merged commit 976f613 into microsoft:main Dec 19, 2022
@Thomas1664 Thomas1664 deleted the comment-layout-fixes branch December 23, 2022 18:10
eslamashour1 pushed a commit to eslamashour1/vscode-pull-request-github that referenced this pull request Dec 31, 2022
* Use bin as delete icon

* Don't remove action bar

* Fix layout

* Revert unrelated format changes

* Fix icon
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.

4 participants