Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

[expand button]: initial PR #129

Merged
merged 2 commits into from
Jun 27, 2017
Merged

[expand button]: initial PR #129

merged 2 commits into from
Jun 27, 2017

Conversation

legomushroom
Copy link
Member

Adds the expand button for HUD:

hud

@msftclas
Copy link

@legomushroom,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@mike-kaufman
Copy link
Contributor

@davidkpiano , @philliphoff - please review this.

@davidkpiano
Copy link
Contributor

Looks great! 🚢

Copy link
Contributor

@mike-kaufman mike-kaufman left a comment

Choose a reason for hiding this comment

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

please get sign-off from David & Phil before mergingin.

@davidkpiano
Copy link
Contributor

@legomushroom If you can, can you narrow the width of the div containing the caret? (maybe by 1/2, cc. @avanderhoorn )

Otherwise looks good to me

@avanderhoorn
Copy link
Member

Feedback on design... can we make it a bit more narrow? By about 1/3 or 1/2?

@legomushroom
Copy link
Member Author

@avanderhoorn

1/3:

image

image

@avanderhoorn
Copy link
Member

Thats better! And what does 1/2 look like?

@legomushroom
Copy link
Member Author

@avanderhoorn

1/2:

image

image

@avanderhoorn
Copy link
Member

I like the last best. Can the collapsed carrot be hollow (like in VS Codes find/replace component)?

@legomushroom
Copy link
Member Author

legomushroom commented Jun 27, 2017

@avanderhoorn only if we are ok to pull in svg icon for that, right now we use a vanilla CSS arrow?

@mike-kaufman
Copy link
Contributor

I want to land this in the next ~10 minutes to start a build, so if any styling changes are going to take longer than that, let's open separate bugs to track them.

@philliphoff
Copy link
Contributor

I'm not crazy about the icon choice; it's not obvious to me what the expand/collapse icon mean in this context. To me, using + and - might have been more clear, or icons that are clear arrows. Otherwise LGTM.

@mike-kaufman
Copy link
Contributor

I'm not crazy about the icon choice; it's not obvious to me what the expand/collapse icon mean in this context. To me, using + and - might have been more clear,

This is good feedback, oleg plz open a seperate bug to track icon choice here. I don't want to block the functionality any longer.

@mike-kaufman mike-kaufman merged commit 5ff5faf into dev Jun 27, 2017
@davidkpiano
Copy link
Contributor

I'm not crazy about the icon choice; it's not obvious to me what the expand/collapse icon mean in this context. To me, using + and - might have been more clear, or icons that are clear arrows.

For context, this is modeling the expand/collapse behavior and UI in VS Code (find/replace) for now, but yeah, we can revise this.

@philliphoff
Copy link
Contributor

philliphoff commented Jun 27, 2017

For context, this is modeling the expand/collapse behavior and UI in VS Code (find/replace) for now, but yeah, we can revise this.

I'd say it still doesn't work great in VS Code, but at least works slightly better since it's not expanding to a large panel. I think these icons would be more obvious:

screen shot 2017-06-27 at 11 21 52 am
screen shot 2017-06-27 at 11 21 00 am

@legomushroom
Copy link
Member Author

@davidkpiano @philliphoff I created an issue to track this: #133

@avanderhoorn avanderhoorn deleted the legomushroom-expand-icon branch July 14, 2017 21:47
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.

6 participants