Enable slider values update from code #725
Conversation
|
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
Update demo example
| </Content> | ||
| <SourceContent> | ||
| <BlazorFiddle Template="MatBlazor" Code=@(@" | ||
| <BlazorFiddle Template="MatBlazor" Code=@(@" |
There was a problem hiding this comment.
Generally, adding tabulation or other stuff to a PR that doesn't change a whole lot can add noise to the PR. I would generally split up the PR so we are aware that one is simply to clean up the code styling, while the other is to fix/enhance the code
There was a problem hiding this comment.
reverted. my IDE didn't show whitespaces and autoformatted it for me
| <MatTextField @bind-Value="@Value" Label="Value"></MatTextField> | ||
| <MatTextField @bind-Value="@ValueMin" Label="ValueMin"></MatTextField> | ||
| <MatTextField @bind-Value="@ValueMax" Label="ValueMax"></MatTextField> | ||
| <MatSlider TValue="decimal" Value="@Value" ValueMin="@ValueMin" ValueMax="@ValueMax" ValueChanged="@OnValueChanged2"></MatSlider> |
There was a problem hiding this comment.
OnValueChanged2 is a pretty generic name that doesn't indicate what is changing. It may make sense to modify this so the dev knows what value is changing when looking at the variable.
There was a problem hiding this comment.
i replied in this comment: https://github.com/SamProf/MatBlazor/pull/725/files#r501237630
i think it is the same case.
value is named Value so i think OnValueChanged2 is pretty self explanatory, except the suffix of course, but that can be guessed from the fact that there are two methods in tha same file
| public decimal ValueMin { get; set; } = 0; | ||
| public decimal ValueMax { get; set; } = 100; | ||
|
|
||
| public void OnValueChanged2(decimal val) |
There was a problem hiding this comment.
This method is highly ambiguous to any dev coming into the code. It would be cleaner to help clean up the name of the method. Likewise, since this is a one-liner, you could use an expression body method instead for conciseness.
There was a problem hiding this comment.
actually there already is a method named OnValuChanged on L261 so i cannot use that. i cannot possible image better method name in this case since there is a value being changed - nothing more. and i believe that dev would understand that it is a demo page with multiple examples of a given component so i dont personally consider number suffix as a big deal. furthermore, when there are multiple variables named Val, Val2 - Val5 in the same file. i agree that it could be expression bodied method but i wanted to follow existing format.
|
@MarosValter , @Christian-Oleson has some great constructive criticisms / suggestions. Please follow-up with him if you want to make said changes or question them. If not then give him permission to make the commits to your PR so we can move it forward. Thanks. |
Revert style cleanup changes
|
@enkodellc , @Christian-Oleson thank you for having a look. i already answered and/or fixed some of the issues. also there is no problem with giving write permission |
minor formatting
|
@MarosValter, @enkodellc , @Christian-Oleson Thank you all. Great job. Will be soon in production. |
- PR: Enable MatSlider values update from code-behind #725 (Thanks to [MarosValter](https://github.com/MarosValter)) - PR: Bugfix - Task canceled exception #737 (Thanks to [Christian-Oleson](https://github.com/Christian-Oleson)) - PR: Update MatBlazor Packages #738 (Thanks to [Christian-Oleson](https://github.com/Christian-Oleson)) - PR: Cleanup usings, errors, warnings, etc. #739 (Thanks to [Christian-Oleson](https://github.com/Christian-Oleson))
5 participants
Fixes #661 and #332
Whenever bound value updates, slider reflects changes. It works with Value, ValueMin, ValueMax, Step for now