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

Added 'Orientation' parameter for vertical and horizontal layouts #73

Merged
merged 8 commits into from
Jul 15, 2024
Merged

Added 'Orientation' parameter for vertical and horizontal layouts #73

merged 8 commits into from
Jul 15, 2024

Conversation

fhaefele
Copy link
Contributor

@fhaefele fhaefele commented Jul 2, 2024

test_horizontal
example
example2

Copy link
Owner

@bastibe bastibe left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! This is super useful!

However, I don't like the way the plotOrientation function makes the code harder to reason about in some places. I have sketched out an alternative structure that I'd prefer.

Nevertheless, great work, and thank you very much!

Violin.m Outdated Show resolved Hide resolved
Violin.m Show resolved Hide resolved
Violin.m Outdated Show resolved Hide resolved
@fhaefele
Copy link
Contributor Author

fhaefele commented Jul 4, 2024

Hm, merging and resolving conflicts is still pretty new to me. I gave it my best. Also I have addressed all of your comments. Let me know if everything fits now, else I will try to fix it asap. I was also thinking of including that 'parent' property which you discussed in the other PR.

I also don't know whether you saw that I made changes to violinplot.m as well. I hope those are fine as well.

Copy link
Owner

@bastibe bastibe left a comment

Choose a reason for hiding this comment

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

This looks much better, thank you!

Just one more quick note: Could you format the code with spaces after operators (such as commas), so it matches the rest of the code?

Otherwise, this looks good to me!

Violin.m Outdated Show resolved Hide resolved
@fhaefele
Copy link
Contributor Author

fhaefele commented Jul 6, 2024

Should be done and ready to merge. Do you want to also add the 'parent' property from #24 ? Should be an easy inclusion.

@bastibe
Copy link
Owner

bastibe commented Jul 7, 2024

Do you want to also add the 'parent' property from #24 ?

That would be a good idea, thank you!

@fhaefele
Copy link
Contributor Author

fhaefele commented Jul 8, 2024

I have added now the parent property and checked the functionality of the orientation swap and the 'parent' axis in a modified testviolinplot.m. To me it looks like it works fine.

@bastibe
Copy link
Owner

bastibe commented Jul 8, 2024

Looks good to me! Thank you!

If you want to, you could add an example to the README.

Is this ready to merge on your end?

@fhaefele
Copy link
Contributor Author

fhaefele commented Jul 8, 2024

Yeah on my end it should be ready. However, if you want me to add an example in the README, then we should maybe wait with merging. The same goes for the testviolinplot.m. I could add some tests to facilitate any future PR don't create problems with the two new features.

@bastibe
Copy link
Owner

bastibe commented Jul 8, 2024

I'll leave it up to you whether you want to add it to the README.

@fhaefele
Copy link
Contributor Author

fhaefele commented Jul 12, 2024

I have edited the README and the readme_figures.m respectively. The README now shows a code snippet that can work on its own, which the previous one couldn't. I have also added two new test cases into testviolinplot.m which test the 'Parent' and 'Orientation' property. Feel free to merge it into the main branch as you like.

@bastibe bastibe merged commit ada5186 into bastibe:master Jul 15, 2024
@bastibe
Copy link
Owner

bastibe commented Jul 15, 2024

Beautiful! Thank you!

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.

2 participants