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 the Side Plot option for the graph option in the insert dialog #9441

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

derekagorhom
Copy link
Contributor

Fixes partly #8800
I have added the side plot option for the graph option of the Insert dialog.
Currently still working on the Text path option.
@fran2or can you review this
Thanks
This is still not ready for review

Copy link

@fran2or fran2or left a comment

Choose a reason for hiding this comment

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

@derekagorhom I've reviewed your progress, and it aligns with our discussion. However, is there a way to ensure that the insert is placed directly where expected within the script? @rdstern, could you also check this?

Currently, the user must determine the exact location in the code to insert the side plots' code; otherwise, it defaults to the end of the R-Instat graph code, causing it to malfunction.

@rdstern, is this acceptable, considering that users utilizing this feature are expected to have some coding knowledge? If they can read the code to that level, they might not need an automated insert function for side plots—they could simply write the code themselves. Thoughts?

@rdstern
Copy link
Collaborator

rdstern commented Feb 14, 2025

@fran2or I am about to check the dialog. But in answer to your query I was assuming that the user has to understand the script sufficiently to put the cursor manually in the right place. Let's ponder later if we can do anything better.

@rdstern
Copy link
Collaborator

rdstern commented Feb 15, 2025

@derekagorhom and @fran2or this is already working. Great:
a) The current labels for the side plots are X Side and Y Side. For R-Instat users I wonder if Top and Right would be simpler? Up to you.
b) A question for @fran2or as an R whizz. The side plots make use of the aeshetics from the main plot. I suggest we could usefully modify the code generated for the main plot, e.g. the point plot, so when it adds a colour aesthetic it also adds fill for the same variable and v.v.? I suggest this could be general in the plot dialogs. It will not change anything in the main plot and sometimes the side plots could use the other?
c) A more general question for @N-thony. We don't yet have a find-replace facility for scripts. This is discussed in a few places, for example #9026. I wonder if it might be quite a simple job - at least could we check. If it existed, then we could copy the last item from the graph into the dialog,
e.g theme_grey() in my example. This would then be replaced by:
theme_grey() + ggside::geom_xsideboxplot() + ggside::geom_ysideboxplot()

@rdstern
Copy link
Collaborator

rdstern commented Feb 17, 2025

@derekagorhom that's fine. Is there a problem adding the other part?

@derekagorhom
Copy link
Contributor Author

@rdstern I am still working on them, I will let you know when they have been added

@derekagorhom derekagorhom marked this pull request as ready for review February 18, 2025 14:07
@derekagorhom
Copy link
Contributor Author

@rdstern , @fran2or I have added the textpath option
This PR is ready for review

@rdstern
Copy link
Collaborator

rdstern commented Feb 18, 2025

@derekagorhom this is a start. However there remains a lot to correct here in the code, and possibly in the controls. I suggest:
a) You disable this option for now. That is what you had earleir. Then we can approve with the simple, and useful, controls for the side plots.
b) I am keen to get to a stable release very soon, and there are more important tasks than this one. So I will be suggesting other issues.
c) You return to this pull request after the next release and @fran2or will be the main tester of your progress.

@derekagorhom
Copy link
Contributor Author

@rdstern I have disabled the Textpath option

@rdstern
Copy link
Collaborator

rdstern commented Feb 19, 2025

@derekagorhom sorry, I keep messing you up on this. That's where I wish you were doing this with @fran2or as well as with me.
So let's leave it for now - I am beginning to doubt whether it is in the right place, given it works very well just adding a single control.
So, we still have the disabled Side Plot control in the scatterplot dialog. Maybe have 2, so 1 for Sode Plot (Top) and the other for Side Plot (Right).

But that's not quite right, because the side plots are then just for scatterplots. They can be added to any plot. So maybe there is a Sideplot tab in the sub-dialog. And maybe both, so the feature is on the main dialog for scatterplots, and in the sub-dialog for everything else.

There are many other features I would like to add quickly into the forthcoming version. Given the time on this one feature they have higher priority than this one.

@fran2or
Copy link

fran2or commented Feb 20, 2025

A question for @fran2or as an R whizz. The side plots make use of the aeshetics from the main plot. I suggest we could usefully modify the code generated for the main plot, e.g. the point plot, so when it adds a colour aesthetic it also adds fill for the same variable and v.v.? I suggest this could be general in the plot dialogs. It will not change anything in the main plot and sometimes the side plots could use the other?

That's an interesting suggestion! Modifying the main plot code to automatically add fill when colour is specified can be done
and could indeed make it easier for side plots to inherit the aesthetics seamlessly. However, this change would also mean that side plots, such as density plots, would always be filled, removing the option for unfilled density plots.

This could be a limitation in cases where overlapping density plots need to be interpreted, as unfilled plots often provide a clearer visual comparison. Perhaps a more flexible approach could be to give users the option in the dialog—either allowing fill to be added automatically when colour is set or letting them decide manually when needed.

What do you think? Would this balance usability and interpretability?

@fran2or
Copy link

fran2or commented Feb 20, 2025

So, we still have the disabled Side Plot control in the scatterplot dialog. Maybe have 2, so 1 for Sode Plot (Top) and the other for Side Plot (Right).

But that's not quite right, because the side plots are then just for scatterplots. They can be added to any plot. So maybe there is a Sideplot tab in the sub-dialog. And maybe both, so the feature is on the main dialog for scatterplots, and in the sub-dialog for everything else.

@rdstern Is there a specific reason why we need the Side Plot controls on the scatterplot main dialog? If not, I think having it present in the sub-dialog would be the best placement. This way, the Side Plot feature will be available not just for scatterplots but for other plots as well, ensuring broader functionality and then avoids duplication.

@rdstern
Copy link
Collaborator

rdstern commented Feb 20, 2025

Thanks @fran2or for the quick reply. I went through the same steps as you have done.
a) I assumed adding the side plots would be quite complicated, and so thought the script- insert route was the way to go.
b) Then you and @derekagorhom showed we could add them with just a single control. That made me reflect that perhaps the single control could go onto the main scatterplot dialog instead.
c) But, as you say, the side plots can be added to any main plot, so maybe a sub-dialog is better. That could be an easy way also to add the same options for that extra as for the other geoms. But maybe we could have a single "page" for all the geoms, with just the control at the top of the page dictating the exact geom?
d) I noiw wonder about both. Nice to be able to show them, really easily for one type of graph. Then we add them, just from the sub-dialog, for the other geoms.
e) However, this will now take longer than I thought earlier. There are a few tasks that fit well with our current training courses, and which I feel could now take priority, with you maybe returning to this task later in March? In particular I hope @derekagorhom and you would find at least one of #9440, #9450 or even #9447/#9451 to your liking. Happy to chat about them if you wish.

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