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

Updated Boxplot dialogue's Plot Options to the new system. #7570

Merged

Conversation

EstherNjeriLiberatta
Copy link
Contributor

@EstherNjeriLiberatta EstherNjeriLiberatta commented Jun 27, 2022

Fixes #7556
In this PR, I have replaced the Box Options button in the Box plot dialogue with the new Plot Options button which includes a drop-down.
@rdstern @africanmathsinitiative/developers This is ready for review.

@EstherNjeriLiberatta EstherNjeriLiberatta changed the title Updated Box Plot dialogue's Plot Options to the new system. Updated Boxplot dialogue's Plot Options to the new system. Jun 27, 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.

@N-thony this is great. I have 2 small cosmetic points - but please only if you agree. I am not so good on these aspects.
I wonder if the width of the Options button should be the same as the width of the selector? And aligned with it.
Could there be a horizontal line below the first entry?#

What do you think. This would apply to each of them.

It does make the dialogue look simpler, which is a good thing.

@N-thony
Copy link
Collaborator

N-thony commented Jul 2, 2022

@N-thony this is great. I have 2 small cosmetic points - but please only if you agree. I am not so good on these aspects. I wonder if the width of the Options button should be the same as the width of the selector? And aligned with it. Could there be a horizontal line below the first entry?#

What do you think. This would apply to each of them.

It does make the dialogue look simpler, which is a good thing.

@rdstern I agrre with you, @EstherNjeriLiberatta please, can you make these changes?

@EstherNjeriLiberatta
Copy link
Contributor Author

EstherNjeriLiberatta commented Jul 13, 2022

@N-thony this is great. I have 2 small cosmetic points - but please only if you agree. I am not so good on these aspects. I wonder if the width of the Options button should be the same as the width of the selector? And aligned with it. Could there be a horizontal line below the first entry?#

What do you think. This would apply to each of them.

It does make the dialogue look simpler, which is a good thing.

@rdstern Thanks for reviewing, I have made the width of the Options button similar to that of the selector. However, I didn't understand the horizontal line you talked about. Do you have an example dialogue I can look up?

@rdstern
Copy link
Collaborator

rdstern commented Jul 13, 2022

@EstherNjeriLiberatta please check the horizontal line for yourself, as it may not be needed. One example is Model > Fit Model > One Variable - the Test button and the button that says Binomial. But looking at that, I am not sure whether a line is needed, given there are far fewer entries.

@EstherNjeriLiberatta
Copy link
Contributor Author

@rdstern Thank you I have checked it, and I agree with you. The dialogue is ready for review.

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.

@EstherNjeriLiberatta it is doing more than I thought - but also therefore sometimes less!
When I am on boxplot, then I can get the ordinary plot options and boxplot options. I can press violin, but I still get the boxplot options. If I check tufte, then I get tufteboxplots.
I was thinking that I might get what I press, even though it isn't sensible. I am happy that I can't, but is it possible to disable the ones that I can't get?
If I am on boxplot (or tufte), or violin, then I can check the points and this adds the jitter layer. But I can't get it. I should be able to.

@EstherNjeriLiberatta
Copy link
Contributor Author

EstherNjeriLiberatta commented Jul 20, 2022

@rdstern I have disabled the options that are not needed when you click on either button box/Tufte, jitter, or violin. It's possible to get the jitter layer when you check the Add points checkbox however only on the plot options.

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.

@EstherNjeriLiberatta that has been ambitious, to just have those enabled, that relate to the options chosen. It is working well, except that the boxplot and violin plots allow points to be added. When that checkbox is used, it is adding a jitter plot, so that option should then be enabled.

@rdstern rdstern mentioned this pull request Jul 30, 2022
@EstherNjeriLiberatta
Copy link
Contributor Author

@rdstern This is ready for review. Thank you.

@EstherNjeriLiberatta
Copy link
Contributor Author

@N-thony Kindly review the changes. Thank you

instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
@EstherNjeriLiberatta
Copy link
Contributor Author

@N-thony Thank you for reviewing. I have made the changes you requested.

Copy link
Collaborator

@N-thony N-thony left a comment

Choose a reason for hiding this comment

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

Great, @rdstern over to you

instat/dlgBoxPlot.vb Show resolved Hide resolved
instat/dlgBoxPlot.vb Show resolved Hide resolved
Comment on lines +385 to 389
If clsCurrGeomFunction.GetParameter("varwidth") IsNot Nothing Then
If clsCurrGeomFunction.GetParameter("varwidth").strArgumentValue = "TRUE" Then
End If
Else
'chkVarwidth.Checked = False
End If
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't seem to do anything. Is it needed?

instat/dlgBoxPlot.vb Show resolved Hide resolved
sdgLayerOptions.ShowDialog()
bResetBoxLayerSubdialog = False
'Coming from the sdgLayerOptions, clsRgeom_boxplot and others has been modified. One then needs to display these modifications on the dlgBoxPlot.
If clsCurrGeomFunction.GetParameter("varwidth") IsNot Nothing Then
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above

instat/dlgBoxPlot.vb Show resolved Hide resolved
Comment on lines +445 to +446
'This resets the factor receiver and causes it to be cleared of the correct variable. We don't want this.
'ucrVariablesAsFactorForBoxplot.SetReceiverStatus()
Copy link
Contributor

Choose a reason for hiding this comment

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

delete?

Suggested change
'This resets the factor receiver and causes it to be cleared of the correct variable. We don't want this.
'ucrVariablesAsFactorForBoxplot.SetReceiverStatus()

instat/dlgBoxPlot.vb Show resolved Hide resolved
instat/dlgBoxPlot.vb Show resolved Hide resolved
instat/dlgBoxPlot.vb Show resolved Hide resolved
@lloyddewit
Copy link
Contributor

@N-thony is @EstherNjeriLiberatta available to address the comments above, or should we reassign?
thanks

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.

This is much closer than the other version.

@N-thony please check whether @EstherNjeriLiberatta has the time to do this quickly. If not, then please re-allocate.

  1. Here is the current version in 0.7.7
    image
    Notice that the new version has lost the Add Points row of controls.
    That should be returned.
    And when it is ticked, then also enable Jitter as well as Box, because it is adding a layer
    b) Add a Summary option into the pull-down. When the Groups to Connect is ticked, then stat-summary is enabled.
    c) And change Box to Boxplot.

@lloyddewit lloyddewit merged commit 391b65b into IDEMSInternational:master Feb 14, 2023
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.

Change the specific plots to the new system
4 participants