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

How to show and hide a split view pane with mvvm guide #537

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Nemonek
Copy link

@Nemonek Nemonek commented Sep 2, 2024

Hey, I've been looking around the Avalonia docs for a while and I spotted this guide with written "to do". I kept an eye on it for a while and I decided to write it. So here's my pull requeste. I hope everything is in order.


## Adding content to the button
Finally, you now just need to add some content to the button that indicates whether you need to show or hide the SplitView's pane. In this guide we will be using the following characters:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the button really need to have it's content changed? It seems somewhat superfluous to the main point of the guide, which is showing how to control the open/close state of the split view.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's something people that looks around for this guide might want to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be its own separate section then as an "advanced" thing. Otherwise it's just conflating two different concepts together.

Copy link
Author

Choose a reason for hiding this comment

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

By separate section do you mean in the same document or another one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either is fine with me. One of the Avalonia devs may say something else.

@thevortexcloud
Copy link
Contributor

thevortexcloud commented Sep 6, 2024

I have added some things I think should be addressed. But to be clear: I am not an Avalonia dev so it's also not really up to me either.

@Nemonek
Copy link
Author

Nemonek commented Sep 6, 2024

I have added some things I think should be addressed. But to be clear I am not an Avalonia dev so it's also not really up to me either.

Hey, thank you so much for the time! This is my fisrt time trying to contribute to a repository, and every advice or constructive criticism is always welcome!
I already fixed/changed the most of what you addressend, except for the major things that I will fix tomorrow.
Feel free to review it and point out other problems and/or corrections you think should be done. I will write another comment once I completed what remains to be done.

Just one question: to make a framework agnostic approach do you think it's ok to have a section with Using ReactiveUI and another with "Using INotifypropertyChanged"?

Some things still needs to be checked and fixed, but the majority is done. Or at least it should be.
@thevortexcloud
Copy link
Contributor

thevortexcloud commented Sep 6, 2024

Hey, thank you so much for the time! This is my fisrt time trying to contribute to a repository, and every advice or constructive criticism is always welcome!

Thank you for the PR, and welcome. It's appreciated!

Just one question: to make a framework agnostic approach do you think it's ok to have a section with Using ReactiveUI and another with "Using INotifypropertyChanged"?

That's fine with me.

@timunie
Copy link
Contributor

timunie commented Sep 9, 2024

Hello @Nemonek

first of all thank you for the contribution. However I have one idea to make this sample even easier to explore. We have https://github.com/AvaloniaUI/Avalonia.Samples where we want to create tutorials and detialed samples. These can be linked from the docs. Benefit is that anyone can just grab the source, download it and play around. From what I see your sample would fit perfectly here. What do you think?

…matical errors. Clarified the namespace usage in the code.

To do the part with the framework agnostic approach.
@Nemonek
Copy link
Author

Nemonek commented Sep 9, 2024

Alright, so, I fixed some things that were to be done, and I also added the parts with the framework agnosti approach as suggested.
Hello @timunie and thank you for the proposal!
I think it would be a good idea and I'd love to do so! Just some questions though, as I said I know have the code with both Reactive UI and without. Should I upload the one with Reactive UI, the one without or both?
Also, should I rename the project from How to show and hide ecc... to like "Show and hide a split view pane with MVVM"?

@timunie
Copy link
Contributor

timunie commented Sep 9, 2024

I think both would be fine. Make it a draft PR for now as I may want to feedback about the sample beforehand. I'm thinking about a Hamburger Menu like solution for the demo, could be under custom controls.

What I don't understand is why we use a button instead of a togglebutton. A toggleButton can just be bound to IsChecked and we are there without much hassle. This is why I want to see your sample life to understand what may improve.


## Adding content to the button
Finally, you now just need to add some content to the button that indicates whether you need to show or hide the SplitView's pane. In this guide we will be using the following characters:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be its own separate section then as an "advanced" thing. Otherwise it's just conflating two different concepts together.

@Nemonek
Copy link
Author

Nemonek commented Sep 10, 2024

Ok, after playing a bit with Github desktop I managed to fork the repository. Anyway, I uploaded both the projects in the same directory as the main solution, as I don't know if you want to put those in the MVVM folder or another. Here's the link.
I also renamed both projects, as I firstly copy pasted the project with reactive UI on my pc and just removed it from the copy.

For what regards the toggle button, well, I hadn't tought of that; sure it would be better and especially simpler to use that, as there wouldn't even be the need for a command. Just a simple binding. Let me know what should be modified and I'll take care of it!


## Adding content to the button
Finally, you now just need to add some content to the button that indicates whether you need to show or hide the SplitView's pane. In this guide we will be using the following characters:
Copy link
Contributor

Choose a reason for hiding this comment

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

Either is fine with me. One of the Avalonia devs may say something else.

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

Successfully merging this pull request may close these issues.

3 participants