-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
Fixed the markup extension for the SymbolIcon and FontIcon class not working at all. #1067
Conversation
…o easier scale the arc aswell as a new startpoint at 12am and refactoring of the code.
@pomianowski I have no idea why this commit went into this pull request :s So: As the arc class wasn't quite working so i thought "Why not? I do need it anyways.". In the course of this, I thought it would be better for the user if he had the opportunity to edit both the sweepdirection and the penlinecap. I also changed the starting position, from 3 a.m. to 12 a.m. I'll be happy to explain the reason for this: If you don't agree and/or are satisfied with it, it can always be changed^^ |
src/Wpf.Ui/Controls/Arc/Arc.cs
Outdated
{ | ||
#region Declarations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to not use regions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove them if you want but i never understood why anyone shouldn't use them. Can you tell me why you don't want them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purely stylistically, I think they are code smell. They hide part of the code because it is difficult to read, and if the code is difficult to read, it means that it is not good code. It is known that WPF UI needs a lot of work to improve readability and cleanliness, but it is worth working on it
namespace Wpf.Ui.Controls; | ||
|
||
/// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removing docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well i removed them for a better overview and just forgot to add them back.
@m0lDaViA I found a new "feature" while using the NavigationView in LeftFluent mode. If you are using a different Font like Montserrat (which is actually quite big), you end up with a cut-off string, even if you increase the item size manually. I think it might be the ContentPresenter... |
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Currently you cannot use a markup extension for symbolicons or fonticons because the constructor does not accept the arguments.
What is the new behavior?
The markup extension for the symbolicon and fonticon class are now working with some small issue.
Other information
So, I have now tested this extensively. I had previously closed this thread because for some reason the FontIcon markup extension randomly worked. However, I think that was due to VS (bugs and such).
I also confused the FontIcon class with the SymbolIcon class in the thread (sorry x.x).
So: The markup extension needs a standard constructor without arguments. Furthermore, the existing constructor does not accept 2 arguments.
Because of the bugs in VS, I wasn't sure before whether it was really a mistake in the markup extension
(that's why i closed the last pull request), but now I am. The SymbolIcon markup extension had the same problem.
The font size doesn't (forgot work), but it's not clear to me why.
Maybe you know why @pomianowski ?