Changed mmtable2() to gt() in Describe < Tables dialog #8259
Conversation
|
Great! To update, @anastasia-mbithe I will have a good review of this tomorrow and get back to you then. Do you have something to work on in the meantime? @rdstern I suggest you hold off looking at this PR until after I've reviewed? |
There was a problem hiding this comment.
Really nice job @anastasia-mbithe! Looks great, and works really nicely.
A few minor comments below. And in addition to these:
- The receiver(s) clear on reopen (this occurs for factors and summaries)
- If I check the "Treat Summaries as Further Factor" checkbox, then select the "Variable" rdo, then uncheck the "Treat Summaries as Further Factor" checkbox, there is nothing selected for the rdo buttons. I suggest that if "Variable" or "Summary" is ever checked (and "Treat Summaries as Further Factor" is not checked), then "Summary-Variable" should be checked? (Otherwise, clicking and unclicking the checkbox shouldn't affect the rdo). What do you think?
|
@lilyclements, the first comment was fixed in #8027, but I have fixed it here again to avoid the repetitive bug in case the other pull request takes time before being merged. |
@rdstern I'm concerned that fixing the same issue in 2 different PRs may trigger a merge conflict. PR #8027 hopefully only needs a small regression test from you before we merge. It's been waiting for this test since 31 March so may have slipped under the radar. It would be great if we can merge PR #8027 fairly soon, thanks |
There was a problem hiding this comment.
@anastasia-mbithe and @lilyclements this seems to be working ok. Great. I can think of many improvements I would like, but would like to merge this version, (without (mmtable2) as soon as you feel it can be a replacement. Then it become our new baseline. The next priority will be the simpler job of replacing mmtable2 in the one-variable and 2/3 variable cases. Then we return to this dialogue.
There was a problem hiding this comment.
@anastasia-mbithe this is working great. A really nice job - well done. Happy for this to be merged 🥇
@lloyddewit over to you!
lloyddewit
changed the title
This fixes some of #8248
@lilyclements and @rdstern This is ready for testing.