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 Pair Plot Options sub dialog for Legend on Graphs dialog #8005

Merged
merged 8 commits into from
Jan 9, 2023

Conversation

MeSophie
Copy link
Contributor

@MeSophie MeSophie commented Dec 5, 2022

Fixes #7212
@rdstern I added legend on Graphs dialog for Pairs radio button.
@N-thony Please can you have a look.

@rdstern
Copy link
Collaborator

rdstern commented Dec 5, 2022

@MeSophie the Options button On Describe > Two/Three Variables > Graph > Pairs is still disabled. So I have not been able to check.

@N-thony
Copy link
Collaborator

N-thony commented Dec 6, 2022

@MeSophie the Options button On Describe > Two/Three Variables > Graph > Pairs is still disabled. So I have not been able to check.

@rdstern I checked this and it seems to work fine
image
@MeSophie I noticed now we lost the Plot Options button for the By option, but Pair Plot Option seems to work fine
image
image

@N-thony
Copy link
Collaborator

N-thony commented Dec 6, 2022

@anastasia-mbithe can you peer-review this?

@MeSophie
Copy link
Contributor Author

MeSophie commented Dec 6, 2022

@rdstern the Plot Options is visible for By now.

@rdstern
Copy link
Collaborator

rdstern commented Dec 8, 2022

@MeSophie this is already looking nice.
a) I noticed that Ok is only enabled when you include the Colour (Optional). It should be included as soon as the pairs can be used. Possibly the Options button should only be enabled when the colour is completed?
b) Do we need 2 controls in the sub-dialogue? I am happy with that, but perhaps Legend could be just a label, with None as the default? I am happy either way though.

@MeSophie
Copy link
Contributor Author

MeSophie commented Dec 8, 2022

@MeSophie this is already looking nice. a) I noticed that Ok is only enabled when you include the Colour (Optional). It should be included as soon as the pairs can be used. Possibly the Options button should only be enabled when the colour is completed? b) Do we need 2 controls in the sub-dialogue? I am happy with that, but perhaps Legend could be just a label, with None as the default? I am happy either way though.

@rdstern The OK is only enabled when you include the Colour (Optional) because if you don't include it and use legend you will have the following error.

image

And For the two controls in the sub-dialog the first one can be enough the second one is just for cosmetic to let the user put the legend wherever he want.

@rdstern
Copy link
Collaborator

rdstern commented Dec 8, 2022

@MeSophie but that's why I would strongly prefer you (instead) activate Ok without needing to add the optional factor. But disable the Options, or the legend instead!

Ok with the 2 controls. But I assume now (with your options) you could tick the first one to have a legend, and then set it to none? That's odd.

@N-thony
Copy link
Collaborator

N-thony commented Dec 14, 2022

@MeSophie but that's why I would strongly prefer you (instead) activate Ok without needing to add the optional factor. But disable the Options, or the legend instead!

Ok with the 2 controls. But I assume now (with your options) you could tick the first one to have a legend, and then set it to none? That's odd.

@MeSophie any progress?

@MeSophie
Copy link
Contributor Author

but that's why I would strongly prefer you (instead) activate Ok without needing to add the optional factor. But disable the Options, or the legend instead!

@rdstern according to @N-thony comment I think the dialog and sub-dialog are better now. I remove the first check box on sub dialog and made Pairs Plot Option sub dialog enable when we fill the Colour Factor.

rdstern
rdstern previously approved these changes Dec 29, 2022
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@MeSophie looks good. Thanks. I am approving

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

@MeSophie thanks, looks good, just a couple of questions

instat/dlgDescribeTwoVarGraph.vb Outdated Show resolved Hide resolved
instat/dlgDescribeTwoVarGraph.vb Outdated Show resolved Hide resolved
@MeSophie
Copy link
Contributor Author

MeSophie commented Jan 4, 2023

@MeSophie thanks, looks good, just a couple of questions

@lloyddewit I revolved your comments thank you.

@lloyddewit lloyddewit changed the title Added Pair Plot Options sub dialog for Legend on Graphs dialog. Added Pair Plot Options sub dialog for Legend on Graphs dialog Jan 9, 2023
@lloyddewit lloyddewit merged commit 87b3682 into IDEMSInternational:master Jan 9, 2023
@rdstern
Copy link
Collaborator

rdstern commented Jul 18, 2023

@lloyddewit I am not sure what should happen if I click on revert above?

I would like to reopen this pull request, or issue, by @MeSophie . This is for the following reasons.
a) In Version 0.7.16 the Pairs Plot Options is disabled. I thought this was the purpose of this pull request.
b) Our main plotting features have regressed again in a small way, and I wonder if this issue/pull request is related. Namely a useful option in the plotting sub-dialogue is shown here:

image

The default option here used to be None - so to be able to not have a legend. This has disappeared! I don't know if this is related to this pull request, or not?

@lloyddewit
Copy link
Contributor

I am not sure what should happen if I click on revert above?

@rdstern This would create a new PR with proposed changes that would undo the specific changes in this PR.
We don't know for certain what caused the regression. So a full revert may not be the best option. I suggest that we start with the latest master version and investigate the bug. I assume that @MeSophie is the best person for this.

@MeSophie
Copy link
Contributor Author

@MeSophie I investigate on this and my question is Do you need the Pairs Plot Options is disabled or enable? because from now it is enable only when you fill the colour receiver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ggpairs to the Describe > Two Variables > Graphs dialogue
4 participants