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

Changed underlying R function in dlgOneVariable Summarise > customised tab (from mmtable function to gt function) #8330

Merged
merged 9 commits into from
May 23, 2023

Conversation

anastasia-mbithe
Copy link
Contributor

This is a continuation of issue #8248
@rdstern and @lilyclements, This 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.

@anastasia-mbithe this is great.
a) I am not sure if you changed the skim option, but it now does seem to provide all the numeric summaries on a single line, which is great.
b) It includes the Summaries button which I assume should be hidden.
c) On the customised could there be a group box with a label Rows: Then I suggest the first (default) option might be Variables, see below. The other two options are just as you have them now, namely Variables and (lastly) None.

image

d) Could there be another option Display Missing as: , with a drop down for options, and the facility to type youe own. NA is the (visible) default followed by (blank) which ideally is written like that, but gives a blank. Otherwise it is just spaces. Then perhaps . (dot) or ... is another option and --- might also be provided?

Copy link
Contributor

@lilyclements lilyclements left a comment

Choose a reason for hiding this comment

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

@anastasia-mbithe this is looking great - I have nothing to add. Let me know once Roger's suggestions are implemented and I'll re-review. Nice one!

@anastasia-mbithe
Copy link
Contributor Author

@rdstern, I didn't work on the skim option but am glad it's fine.
On point (c.) should the group label be Rows yet the options specify the factors to be used as column names?
On the last point, I have added the facility to replace NA/missing with the options you suggested, however, that is not working at the moment and @lilyclements has promised to work on it (the parameter) from the function level separately.

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.

@anastasia-mbithe that's looking very nice now.
You are quite right - only an idiot (me) would say Rows when it should be Columns!
And a tiny detail - I can't see exactly whether the Options button is lined up with the Omit Missing Values checkbox, or is slightly low?

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.

@anastasia-mbithe this is looking very nice now. Here is an example, when I made fert into an ordered factor! It shows that then there is a min and a max - which you don't have for an ordinary factor. It even calculates the median - I didn't realise it could do that!

image

I hope it might now be easy to add 2 controls from the general summary tables dialogue

image

This would make it more useful, and it can also be a teaching tool. What is a table? How can we make it more attractive, and so on!

And could you also delete the As from the Display Missing As. I think having the As there in English is sort of ok, but it could be tricky to translate.

@rdstern
Copy link
Collaborator

rdstern commented May 20, 2023

@lilyclements and @anastasia-mbithe could you please also check the code you are using? It is all working swimmingly with my little survey which has 36 rows. Howver, when I tried with Dodoma and then a trivial summary of one variable (carat) and default summary for the ggplot2 diamonds data, I always had to break out. It never finished.

Oh and I wonder about the Margins option in the dialog? Unless I hav emisunderstood, I suggest it can be deleted.

Sorry - I also keep having more things. On the Default button we have changed so the Maximum factor levels is 12 and that's still fine. What's not fine is that you can't make it less than 12! Could you please change the minimum there to be 1. Thanks.

@anastasia-mbithe
Copy link
Contributor Author

@rdstern, I have tested the dialog severally using Dodoma and Diamonds datasets, it gets to give an output, only that there is a lag for almost a minute, for the Diamonds data (maybe due to the dataset's size).
image

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.

@anastasia-mbithe maybe you were not ready for me to test again. I got build errors.

@rdstern
Copy link
Collaborator

rdstern commented May 22, 2023

@anastasia-mbithe working now ok

@anastasia-mbithe
Copy link
Contributor Author

It's ready now, I just had to update the branch, for the sub-dialog functions to be consistent.

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.

@anastasia-mbithe This development is exciting.

a) I occasionally get the developer error when pressing the Format button

image

b) The Format code seems to be missing! I don't get a change of theme, or a title, when I ask for them.

c) Could you make the 2 labels wider for the French text. That's the Omit Missing Values and Save Summary. (Come to think of it, you are making it into a table, so I wonder about Save Table instead? Though if it is the same control for all the top options, then leave it.)

d) I would very much like to merge this in the version this week. If you are still waiting for the Missing options, then perhaps disable for now, so it can be merged.

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.

@anastasia-mbithe this is looking great now. What a teaching tool we are getting towards! @lloyddewit over to you

@lloyddewit lloyddewit changed the title Changed mmtable function to gt function in dlgOneVariable Summarise > customised tab Changed underlying R function in dlgOneVariable Summarise > customised tab (from mmtable function to gt function) May 23, 2023
@lloyddewit lloyddewit merged commit 056d2e7 into IDEMSInternational:master May 23, 2023
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.

4 participants