-
Notifications
You must be signed in to change notification settings - Fork 102
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
Ensured that Bar Chart dialog opens with the correct settings, and improved formatting #8316
Ensured that Bar Chart dialog opens with the correct settings, and improved formatting #8316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items d,e and g look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekagorhom this is looking good. However, while you are there I looked at the translation into French. Adding better width of labels would be good at the same time.
And @MeSophie could you please edit the database to improve the translation of a Pie chart - which is Tart and should be Camembert and also Donut seems to be Anneau. @derekagorhom please make sure the labels have enough space for these.
- @derekagorhom you see the Reorgansier la - that (In English) is
Reorder Frequencies
I suggest you get rif of the Frequencies here (Like the simple Reorder lower down. Then the French will automatically be complete/ - Towards the bottom make the label wider for the Save control label - there is plenty of space.
- For the Pie (Tart) just make sure that the label is wide enough for when Sophie has added the correct Camembert translation.
@rdstern I have made layout changes you requested, but for Item a), @lloyddewit to make database change in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekagorhom Here is a small problem, with the add.factor addition, so it can handle numeric data better.
# Code generated by the dialog, Bar Chart
diamonds <- data_book$get_data_frame(data_name="diamonds")
last_graph <- ggplot2::ggplot(diamonds, mapping=ggplot2::aes(x=forcats::fct_infreq(f=as.factor("carat")), fill="")) + ggplot2::geom_bar(stat="count") + theme_grey()
data_book$add_object(data_name="diamonds", object_name="last_graph", object_type_label="graph", object_format="image", object=check_graph(graph_object=last_graph))
data_book$get_object_data(data_name="diamonds", object_name="last_graph", as_file=TRUE)
In the code where you added as.factor, the variable is in quotes, here "carat". Please delete the quotes.
instat/dlgBarAndPieChart.vb
Outdated
Private Sub ucrPnlOptions_ControlValueChanged(ucrChangedControl As ucrCore) Handles ucrVariablesAsFactorForBarChart.ControlValueChanged, ucrReceiverX.ControlValueChanged, ucrReceiverLabel.ControlValueChanged, ucrReceiverByFactor.ControlValueChanged, ucrPnlOptions.ControlValueChanged, ucrNudMaxSize.ControlValueChanged, ucrInputReorderX.ControlValueChanged, ucrInputReorderValue.ControlValueChanged, ucrInputAddReorder.ControlValueChanged, ucrChkReorderValue.ControlValueChanged, ucrChkLollipop.ControlValueChanged, ucrChkIncreaseSize.ControlValueChanged, ucrChkAddLabelsText.ControlValueChanged | ||
|
||
End Sub | ||
|
||
Private Sub ucrReceiverByFactor_ControlContentsChanged(ucrChangedControl As ucrCore) Handles ucrReceiverByFactor.ControlContentsChanged, ucrPnlOptions.ControlContentsChanged | ||
|
||
End Sub | ||
|
||
Private Sub ucrReceiverX_ControlContentsChanged(ucrChangedControl As ucrCore) Handles ucrReceiverX.ControlContentsChanged | ||
|
||
End Sub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are no codes in those three subs can add the codes if needed otherwise remove them?
instat/dlgBarAndPieChart.vb
Outdated
@@ -807,7 +808,7 @@ Public Class dlgBarAndPieChart | |||
Case strDescending | |||
clsBarAesFunction.AddParameter("fill", clsRFunctionParameter:=clsForecatsInfreqValue, iPosition:=1) | |||
Case strReverse | |||
clsForecatsReverseValue.AddParameter("f", ucrReceiverByFactor.GetVariableNames(False), iPosition:=0) | |||
clsForecatsReverseValue.AddParameter("f", ucrReceiverByFactor.GetVariableNames(False) & Chr(34), iPosition:=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this line, are you trying to wrap the variable from the receiver with quotes?
my understanding from this line is if the variable name is for example x
so you will have f = x"
, is that correct?
instat/dlgBarAndPieChart.vb
Outdated
clsForecatsInfreq.AddParameter("f", "as.factor(" & Chr(34) & ucrVariablesAsFactorForBarChart.GetVariableNames(False) & Chr(34) & ")", iPosition:=0) | ||
clsForecatsInfreqValue.AddParameter("f", "as.factor(" & Chr(34) & ucrReceiverByFactor.GetVariableNames(False) & Chr(34) & ")", iPosition:=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.GetVariableNames as a parameter bWithQuotes
a boolean where if true it will return the parameter name with quotes for example if the parameter is village
you will have if .GetVariableNames(True) f = "village"
and false if you don't want it with quotes
so, you can just have clsForecatsInfreq.AddParameter("f", "as.factor(" & ucrVariablesAsFactorForBarChart.GetVariableNames(False) & ")", iPosition:=0)
@derekagorhom can you make the checkbox |
|
@N-thony I corrected the translation of |
@MeSophie everything is approved already, I think @lloyddewit need to merge the new PRs created to update the database with new translations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekagorhom well done - these now all seem to work. It would be good to merge. There will continue to be improvements, I am sure! Over to you @lloyddewit
@MeSophie Thank you for the CrowdIn changes. I accepted/approved them all. I also changed |
Fixes partly #8288
This PR is ready for review
@rdstern i was able to fix items d, e, and g.
Item f will be fixed along with the new ones @COLIEWO found recently