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.

Sign up for GitHub

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

Jump to bottom

Inheritance Margin UI #52145

Merged
merged 33 commits into from
Apr 2, 2021
Merged

Inheritance Margin UI #52145

merged 33 commits into from
Apr 2, 2021

Conversation

Cosifne
Copy link
Member

@Cosifne Cosifne commented Mar 25, 2021 •
edited
Loading

Add the UI for inheritance margin.

A general look:
inheritanceMargin9

If there is one member on the line:
image

If there are multiple members on the line:
image

Also works for VB
image

Works with the theme in VS (Using the dark theme)
image

If there are a lot of items in the pop up, it would use a scrollview to view them
inheritanceMargin7

@CyrusNajmabadi
Copy link
Member

Why do those pictures look so weird? :-/

@Cosifne
Copy link
Member Author

Cosifne commented Mar 26, 2021

Why do those pictures look so weird? :-/

That's why it is still WIP : )

@Cosifne Cosifne changed the title (WIP) Inheritance Margin UI Inheritance Margin UI Mar 26, 2021
Cosifne
Cosifne commented Mar 26, 2021
View reviewed changes
src/EditorFeatures/Core.Wpf/InheritanceMargin/MarginGlyph/InheritanceMargin.xaml
<imaging:CrispImage x:Key="NonSharedIcon" x:Shared="False" Moniker="{Binding ImageMoniker}"/>

<!-- Template copied from editor -->
<ControlTemplate x:Key="ContextMenuTemplate" TargetType="{x:Type ContextMenu}">
Copy link
Member Author

@Cosifne Cosifne Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For whoever review the xaml, I copied some styles and control template from the editor light bulb style (which I believe orginally it is copied from shell), modified a little bit in order to fit our needs.

I choose this way because when I try to set the theme for each component in the context menu, I realized it would require breaking into the inner grid in the ControlTemplate, setting everything from scratch, and later on, I feel this is what the light bulb has done, and they also have proper margin and other property set aleady.

I really don't like this approach, so I am open to any other idea to implement this, if you know any vsct file magic or there is any good shell library that I don't know, please tell me and I am super happy to change this part.

The needs are:

  1. The MaxHeight could be set for the context menu or scrollViewer. So that if there are a lot of item in it, the.
  2. The context menu and menu item needs to be correctly themed according to VS.
  3. The context menu needs to be dynamically popped up.
  4. Submenu is needed.
  5. It needs to be shown when clicking the margin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I think the only thing to simplify would be to have styles inherit from known resource keys: https://docs.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.shell.vsresourcekeys?view=visualstudiosdk-2019

I don't think it covers all of our needs though, and some customization would still be necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Look into the doc and I feel the only usable resource key is scroll viewer. I have add that.

@Cosifne Cosifne marked this pull request as ready for review March 26, 2021 08:12
@Cosifne Cosifne requested review from a team as code owners March 26, 2021 08:12
@sharwell sharwell changed the base branch from feature/inheritanceMargin to features/inheritanceMargin March 30, 2021 17:10
@Cosifne
Copy link
Member Author

Cosifne commented Mar 31, 2021

@CyrusNajmabadi @ryzngard
This is ready to be reviewed again : )
Please check the updated gif and screenshot to see if we need polish any other UI

@ryzngard
Copy link
Contributor

@Cosifne have you done an accessibility pass for this? Need to run https://accessibilityinsights.io/ on various UI elements to make sure no major errors

@CyrusNajmabadi
Copy link
Member

That gif is looking nice :)

@Cosifne
Copy link
Member Author

Cosifne commented Apr 1, 2021

@Cosifne have you done an accessibility pass for this? Need to run https://accessibilityinsights.io/ on various UI elements to make sure no major errors

@ryzngard

The margin:
image

image

Context menu:
image
image

@Cosifne Cosifne force-pushed the dev/shech/inheritanceMarginUI branch from 9983629 to 2d4edce Compare April 1, 2021 18:49
ryzngard
ryzngard approved these changes Apr 1, 2021
View reviewed changes
Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Once the telemetry is fixed everything looks good to me :shipit:

This was referenced Apr 1, 2021
Closed
Closed
Closed
@Cosifne Cosifne merged commit c5ff0d1 into dotnet:features/inheritanceMargin Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikadumont mikadumont mikadumont left review comments

@ryzngard ryzngard ryzngard approved these changes

@CyrusNajmabadi CyrusNajmabadi Awaiting requested review from CyrusNajmabadi

@sharwell sharwell Awaiting requested review from sharwell

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Cosifne @CyrusNajmabadi @ryzngard @sharwell @mikadumont