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

Default MenuItem style in VS2013 theme changed #191

Closed
bdachev opened this issue Aug 24, 2020 · 13 comments
Closed

Default MenuItem style in VS2013 theme changed #191

bdachev opened this issue Aug 24, 2020 · 13 comments

Comments

@bdachev
Copy link
Contributor

bdachev commented Aug 24, 2020

There is a set of commits done by LyonJack in VS2013 theme and in particular Generic.xaml that I don't really like. The default style for MenuItem and some other control types is changed which effectively is applied to any control that is inside the layout. Although I can't give ready solution right now I do believe it should be done in less intrusive way. Maybe use explicit style keys that are applied only to menu items in layout related XAML files.

@Dirkster99
Copy link
Owner

Yes, I found a similar issue with the ContextMenu and ToolTip :-( but the one I found was only an issue in a client application and was never visible in MLibTest or any other directly styled test app :-(

My problem with the MenuItem is that I do not have one with it - unless you refer to the ContextMenu which I already removed from Master as you can see in the linked entry above.

Do you refer to this change or do you have an additional problem? If the later, can you please me a bit more detail about which additional change you refer to and whether we should simply roll-back that part or do a different solution?

Thanks Dirk

@bdachev
Copy link
Contributor Author

bdachev commented Aug 25, 2020

Hi Dirk, the problem can be seen in TestApp if a menu is added to a tool. It was even more severe at 4.3 release.
Here is how main menu looks like:
image
Here is tool menu in v4.3:
image
image

It is now a bit better in master currently but still not OK to me:
image

In our project we might end up branching VS2013 themes and customizing them to our desired outlook.

@Dirkster99
Copy link
Owner

Would it help to roll back to version 4.2?

Otherwise, the MenuItem customization is mainly necessary to make the document docking menu on the right-top handside work - if you see a better way of styling this than in 4.2 (or later) I'll be open to alternative solutions...

@bdachev
Copy link
Contributor Author

bdachev commented Aug 25, 2020

Yes. That's how I understand it so the customization should be constrained to that menu only and that is how other themes are done. Anyway let's not do such radical change as reverting to v4.2. Instead I just fulfilled another PR in which I make that style for MenuItem local to VS2013 theme and then it is applied only to instances of MenuItemEx by default. Let LyonJack check if that is OK for their projects. I still don't like how the document docking menu looks in dark theme but at least with that change other menus in layout content windows are not changed.

@Dirkster99
Copy link
Owner

Dirkster99 commented Aug 25, 2020

Hi @bdachev your suggested change works me - no problem here as far as I can see - I even tested it with a local NuGet in one of my open source apps and it looks OK. Really cool if we can apply simple solutions like this (I am sure I tried something similar before but did not get it to work because some highlighting colors would not work...) :-)

@LyonJack Can you please review the suggested change in PR #192 and let us know whether you see a problem with this change or not?

Thanks Dirk

@LyonJack
Copy link
Contributor

LyonJack commented Aug 26, 2020

@Dirkster99 I will make the style of contextMenu as private,It will be used only in our dock,but not user's style.
However, this kind of modification will bring other effects, for example, the user cannot control the style of the dock to follow the style of the user-defined menu.
Now,we have two selection
1.make the style as private
2.dont set any default style of these controls(menu,tooltip)
which one do you think is more userful for users?

@bdachev
Copy link
Contributor Author

bdachev commented Aug 26, 2020

@LyonJack, IMHO we should not set any default style directly in the theme. At least not for generic controls like MenuItem, ContextMenu or ToolTip. In other AvalonDock themes default style is set only for derived controls like ContextMenuEx. Application wide customizations to default styles should be defined outside AvalonDock theme and it should make use of them by defining its own styles with BasedOn="{StaticResource {x:Type ControlType}}".

@Dirkster99
Copy link
Owner

Dirkster99 commented Aug 26, 2020

@LyonJack I agree with @bdachev
Default styles for Generic controls like MenuItem, ContextMenu, ToolTip should be defined outside of AvalonDock.

This can either be done at:

  1. The application that needs it (eg App.xaml) or in
  2. the style library (eg MahApps.Metro) that is consumed by the Client Application of AvalonDock.

So, in a nutshell: users are still able to control the default styles of ContextMenu, MenuItem, and ToolTip - there is just no out of the box solution which makes sense if you think of the high number of different ways to style each of these Generic controls in any of all the styling libraries available today ...

@LyonJack
Copy link
Contributor

ok,I will remove the default styles

@Dirkster99
Copy link
Owner

Its OK I removed the default styles in the master already as I already tried to comunicate here - in this issue we are looking to make the MenuItem applicable to the ContextMenu only which is also already covered in PR #192 - so there is no immediate todo for you unless you see a better/alternative option to get to the same result...

@Dirkster99
Copy link
Owner

I wrote an article about architecturing with WPF themes some time ago because I noticed there is not much documentation given on the right and wrongs for architecturing with WPF themes - and while the case we have here is not discussed in the article there might be other interesting learnings that I have tried to document: File-System-Controls-in-WPF-Version-III - please have a look and let me know if you have any feedback...

@Dirkster99
Copy link
Owner

@bdachev
@LyonJack
I have accepted the PR #192 - does this resolve this issue or there are other problems that we should address (I have no problem with the current MenuItem style but if you see improvements I am happy to include it)?

@Dirkster99
Copy link
Owner

I consider this resolved with version 4.4. which is released now.

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

No branches or pull requests

3 participants