Display version to popup in the Demo App #3736
Conversation
| @@ -45,6 +45,9 @@ jobs: | |||
| MDIXColorsVersion: ${{ inputs.mdix-colors-version }} | |||
| MDIXMahAppsVersion: ${{ inputs.mdix-mahapps-version }} | |||
|
|
|||
| - name: Generate Version File | |||
| run: echo ${{ inputs.mdix-version }} > src/MainDemo.Wpf/version.txt | |||
|
|
|||
There was a problem hiding this comment.
This feels kinda hacky, but I unfortunately can't suggest a better way because I don't have much experience with pipelines.
| @@ -36,6 +36,7 @@ | |||
| <Button Content="Hello World" /> | |||
| <Button Content="Nice Popup" /> | |||
| <Button Content="Goodbye" /> | |||
| <Button Content="Version" /> | |||
There was a problem hiding this comment.
Not sure if this should also be a button since there is no functionality behind it, but I guess it's so this fits in with the rest of the items (Buttons) above
| @@ -38,6 +38,7 @@ | |||
| <ListBoxItem Content="Hello World" /> | |||
| <ListBoxItem Content="Nice Popup" /> | |||
| <ListBoxItem Content="Goodbye" /> | |||
| <ListBoxItem Content="Version" /> | |||
There was a problem hiding this comment.
Having something app-specific on a page which showcases a Material Design component is debatable, but I don't mind that much
| private void LoadVersion() | ||
| { | ||
| try | ||
| { | ||
| string versionFilePath = Path.Combine(Directory.GetCurrentDirectory(), "version.txt"); | ||
| if (File.Exists(versionFilePath)) | ||
| { | ||
| string version = File.ReadAllText(versionFilePath).Trim(); | ||
| VersionText.Text = $"Version: {version}"; | ||
| } | ||
| else | ||
| { | ||
| VersionText.Text = "Version file not found"; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| VersionText.Text = $"Error reading version: {ex.Message}"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Though the demo app isn't there to showcase best practices/design patterns, I would argue being consistent and binding a Version property in a MVVM way, rather than having all that logic in the code behind, is preferred.
There was a problem hiding this comment.
Thanks for comments, yoj mean that is preferable to have a class only to load the version and link it to the MVVM class? Did I understand correctly?
|
Hey @amina-sab, while reviewing your PR, I'd suggest the following code changes: I added some suggestions as a cloud patch (let me know if you have any trouble viewing it). Some key changes here:
You can also review and apply these suggestions locally on your machine. Learn more about GitKraken Code Suggest
Join your team on GitKraken to speed up PR review. |
Related to the issue #3734
Review please @corvinsz @Keboo :)