Restore gt_summary and allow for general usage#1004
Conversation
Unit Tests Summary 1 files 37 suites 27m 12s ⏱️ For more details on these failures and errors, see this check. Results for commit ab1c648. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 55370d3 ♻️ This comment has been updated with latest results. |
llrs-roche
left a comment
There was a problem hiding this comment.
Just two high levels comments
| tagList(!!!cs_ui), | ||
| partial_ui, | ||
| # Allow multiple decorators for a single object (table) | ||
| teal::ui_transform_teal_data(ns("decorator"), transformators = select_decorators(decorators, "table")), |
There was a problem hiding this comment.
This might be a "name conflict" but for SME team and other internal users a table is different from a listing.
I am not sure if there are some interest from app-developers on having all transformers as argument so that they only have to copy and paste the module and change the specific part of the gt-related function they want.
There was a problem hiding this comment.
Ok, I kept the naming from the previous. I'll change this to listing
There was a problem hiding this comment.
Should tmc modules have been using this in the tm_l_** convention for listings? too late now though.
There was a problem hiding this comment.
Yes, the templates id use L instead of T:
But if this was done differently I'm sure there were good reasons or maybe this pattern was not established yet.
Note that I'm saying that depending on what the function used the decorator might be for a table or for a listing. I'm not sure what I used previously or what can be more helpful now.
There was a problem hiding this comment.
Changed all to listing, please check it out. Should we rename the name of the modules as well?
Or did you just mean the tbl_listing? if that's the case then I'd argue against it as it break from standard :-\
|
Hi @averissimo, thanks for your work on this! I really like where this is going! First, we have Then, we have three functions that covers the most common functions using
For these three functions, since they're dedicated to a specific function, I think the Also, is it possible to use teal.picks for this? Now that we introduce teal.picks, it make sense to make any new module only teal.picks compatible. |
Co-authored-by: Dony Unardi <donyunardi@gmail.com> Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Dony Unardi <donyunardi@gmail.com> Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
I think it's best, the only upside is to allow reuse of same modules if parameters are similar.
I was reworking this today and will push it after this round of reviews :-) |
@donyunardi and @averissimo, note that some might be interested in some other functions for example I would keep the flexibility to include other functions. Even if basic functions work reproducing the work of some functions on crane will be hard. I think we should try to be open to support them. Probably shouldn't support all of them from the start, but we should keep it flexible enough for them. |
|
@donyunardi @llrs-roche Updated to picks and incorporated all feedback from PR and discussions. |
| by = NULL, | ||
| include = teal.picks::picks( | ||
| teal.picks::datasets(), | ||
| teal.picks::variables(selected = dplyr::everything(), multiple = TRUE) |
There was a problem hiding this comment.
everything() might be too much, would it make sense to select just a single column as default?
The first run with everything is very slow, as well as the default UI doesn't show "Select all / deselect all".
Latter problem can be resolved, but I'm worried about it taking too long to run
There was a problem hiding this comment.
It is the default for gtsummary, so I think this is what users would expect. Note that on github there have been some improvements to speed up gtsummary so the demo could be run with that branch too 😉
|
I see the improvements on the UI, I think now it looks great. |
|
Decorators are implemented :-) Please feel free to test it out! Note: It works fine with decorators, I've updated the example to have one of them. |





Pull Request
tm_gtsummarytotm_tbl_roche_summary#999Changes description
gt-type data callstm_gtsummarywithgtsummarybackendtm_tbl_roche_summarytm_tbl_listing.funargument in favor of static implementation onlyTechnical details
Creates internal functions called
{tm/srv/ui}_gt_templatewhich implement a very flexible approach to arbitrary named arguments (also known as dots...).Name dots are dynamically generated because there are no common parameters between target functions.
data_extract_spec(futureteal.picks::picks()): Print them dynamically on encoding panel. It will use data fromteal.dataobjectchoices_selected(futureteal.picsk::variables(): Print an optional select input on ecoding panelThis assumes that any module needs at least 2 implemented functions
tm_<module_name>srv_<module_name>_partialwith an optionalui_<module_name>_partialThe
partialallows for custom logic and setting custom and optional UI.Note: no need of partial for
crane::tbl_listingas it has no custom logic.tbl_summaryandtbl_roche_summaryneed as we're using radio buttons and custom validation.Below is an example of a partial for
gt_summary::tbl_summary(must return a qenv-like object to keep withtealpractices)Example app
Note:
tm_gtsummary(without validation)teal.widgets@999-tm_gtsummarybranch fortbl_listingmodule to work (it splits the table in 2)