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

Ensure that Bar Chart dialog combo boxes remember their state correctly when dialog reopens #6517

Merged
merged 9 commits into from
Jun 24, 2021

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Jun 8, 2021

Partially fixes #6135
This PR fixes the bug in the two comboxes to remember the states in reopen, and the presentation of the labels.
@africanmathsinitiative/developers @rdstern this is ready for review. 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.

@N-thony that's already much better. The oddity now is that we have the Position control when the option is set to count, but not otherwise.
I suggest the right-hand-side is going to be a bit more complicated anyway, with the multiple options for x or y. So could you move the Position control to the left hand side. It can be above the Flip button and a single line, so the control is just after the label.
This will make the dialogue a bit longer, which is ok. There will be more options to come.

@N-thony
Copy link
Collaborator Author

N-thony commented Jun 9, 2021

@N-thony that's already much better. The oddity now is that we have the Position control when the option is set to count, but not otherwise.
I suggest the right-hand-side is going to be a bit more complicated anyway, with the multiple options for x or y. So could you move the Position control to the left hand side. It can be above the Flip button and a single line, so the control is just after the label.
This will make the dialogue a bit longer, which is ok. There will be more options to come.

@rdstern I'm not sure about it but after looking at the code, it seems to be a reason that I ignore for instance the Position control is visible when the option is set to count otherwise it is Y variable Receiver control which is at the same location with Position control. I wonder if it is good to move those two controls above Flip button. Can we have a quick chat?

@N-thony
Copy link
Collaborator Author

N-thony commented Jun 9, 2021

@rdstern I made the improvement regarding your suggestions in #6135 from item a) to d). Could you test? 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.

@N-thony that is a reasonable start, but there is a long way to go.

  1. In the second option there needs to be the x-variable label and control. You have lost that.
  2. Then there is a lot to do on the R code that is generated. Currently the only difference in the R-code is whether the orientation is vertical or horizontal. The R-code for the value option does not have the "identiry" option any more. You need to examine the different options in the current Version 7.2 dialogue as a start.

@N-thony
Copy link
Collaborator Author

N-thony commented Jun 10, 2021

@rdstern I have improved the code and design regarding your comments above. Could you test the change? 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.

@N-thony excellent, this all seems to be working!

rdstern
rdstern previously approved these changes Jun 10, 2021
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 all seems to be working. Well done!
@lloyddewit there is more to be done, but I suggest it would be good to merge this first.

@lloyddewit
Copy link
Contributor

@Ivanluv Please could you peer review? thanks

@rdstern
Copy link
Collaborator

rdstern commented Jun 12, 2021

@N-thony there seem to be some problems of clearing part of the R-code. I show the sequence that led to an error:
I ran a bar graph with my usual survey data. Here is the command generated - this runs fine:

# Code generated by the dialog, Bar and Pie Chart
Sheet.1 <- data_book$get_data_frame(data_name="Sheet.1")
last_graph <- ggplot2::ggplot(data=Sheet.1, mapping=ggplot2::aes(x=village, fill=variety)) + ggplot2::geom_bar(position="dodge", stat="count") + theme_grey()
data_book$add_graph(graph_name="last_graph", graph=last_graph, data_name="Sheet.1")
data_book$get_graphs(data_name="Sheet.1", graph_name="last_graph")

This ran fine:
Then I loaded a dataset from the standard R library: datasets. The data file is called BOD

Here is the completed dialogue - it is very simple:
image

# Code generated by the dialog, Open Dataset from Library
utils::data(package="datasets", X=BOD)

data_book$import_data(data_tables=list(BOD=BOD))

# Code generated by the dialog, Bar and Pie Chart
BOD <- data_book$get_data_frame(data_name="BOD")
last_graph <- ggplot2::ggplot(data=BOD, mapping=ggplot2::aes(x=Time, y=demand)) + ggplot2::geom_bar(position="dodge", stat="identity") + ggplot2::geom_text(mapping=ggplot2::aes(fill=variety), stat="count") + theme_grey() + ggplot2::theme(axis.text.x=ggplot2::element_text())
data_book$add_graph(graph_name="last_graph", graph=last_graph, data_name="BOD")
data_book$get_graphs(data_name="BOD", graph_name="last_graph")

Here is the error:
image

You see, in the error and also in the code above, that lots of the code remains from the previous graph.

If I first press Reset, then it delivers - instead - the following code:

# Code generated by the dialog, Bar and Pie Chart
BOD <- data_book$get_data_frame(data_name="BOD")
last_graph <- ggplot2::ggplot(data=BOD, mapping=ggplot2::aes(x=Time, y=demand)) + ggplot2::geom_bar(position="dodge", stat="identity") + theme_grey()
data_book$add_graph(graph_name="last_graph", graph=last_graph, data_name="BOD")
data_book$get_graphs(data_name="BOD", graph_name="last_graph")

This runs fine and gives the graph I want.

Else ucrVariablesAsFactorForBarChart.Add(clsParam.strArgumentValue)
End If
End If
Next
Copy link
Contributor

Choose a reason for hiding this comment

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

when go to the Bar Chart options button andReturnwhen I am within the Value radio button, I get the error below
image

@N-thony
Copy link
Collaborator Author

N-thony commented Jun 14, 2021

@Ivanluv thanks for your comment above, I think it is fixed now. Please have a look. Thanks.

@Ivanluv
Copy link
Contributor

Ivanluv commented Jun 21, 2021

@lloyddewit this is good from my end

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.

@lloyddewit I think @Ivanluv is also ok with this.

@shadrackkibet
Copy link
Collaborator

@N-thony could you improve the PR header? Small bug fixes in Bar And Pie chart Dialog is too general. Remember this header will be used to automatically generate release notes. Could you include the details of the bug fixed?

@N-thony N-thony changed the title Small bug fixes in Bar And Pie chart Dialog Bug fixes: ensure comboxes to remember states in reopen of bar chart dialogue Jun 23, 2021
@lloyddewit lloyddewit added the bug label Jun 24, 2021
@lloyddewit lloyddewit merged commit 6007611 into IDEMSInternational:master Jun 24, 2021
@lloyddewit lloyddewit changed the title Bug fixes: ensure comboxes to remember states in reopen of bar chart dialogue Ensure that Bar Chart dialog combo boxes remember their state correctly when dialog reopens Jun 24, 2021
Comment on lines +210 to +212
<data name="rdoPieChart.Enabled" type="System.Boolean, mscorlib">
<value>False</value>
</data>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@N-thony @rdstern could you explain why the "pie-chart" option was disabled in this dialog? I am afraid these changes might have caused regression!

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.

bar charts needs corrections and possibly single_multiple option
5 participants