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

Extend teal transform vignette #1301

Merged

Conversation

vedhav
Copy link
Contributor

@vedhav vedhav commented Aug 8, 2024

Changes:

  1. Adds the section on how to place the transform UI in the custom position inside the module.
  2. Formats the _pkgdown.yml file (No change made to the file, just styling)
  3. Extend the example_module() to handle the selection when datasets change (This can happen when using DDL to change datasets completely, such an example will be added to teal.gallery)

@vedhav vedhav added the core label Aug 8, 2024
@vedhav vedhav requested a review from m7pr August 8, 2024 11:01
@m7pr
Copy link
Contributor

m7pr commented Aug 9, 2024

@vedhav check my proposition for the wrapper that appends the encoding argument of module$ui part with the function needed to show the transformer, for custom position 145f1d9

Maybe you'd like to polish the wrapper a bit? I had issues using append() function, that's why I used the for loop. But maybe you will be more successful

mod$ui
function(id) {
      ns <- NS(id)
      teal.widgets::standard_layout(
        output = verbatimTextOutput(ns("text")),
        encoding = tags$div(
          selectInput(ns("dataname"), "Choose a dataset", choices = NULL),
          teal.widgets::verbatim_popup_ui(ns("rcode"), "Show R code")
        )
      )
    }
<bytecode: 0x000001e87d53f730>
<environment: 0x000001e800f38ad0>
> custom_module_position(mod, position = 1)$ui
function (id) 
{
    ns <- NS(id)
    teal.widgets::standard_layout(output = verbatimTextOutput(ns("text")), 
        encoding = tags$div(ui_transform_data(transform_id(id), 
            transformers), selectInput(ns("dataname"), "Choose a dataset", 
            choices = NULL), teal.widgets::verbatim_popup_ui(ns("rcode"), 
            "Show R code")))
}
<environment: 0x000001e800f38ad0>
> custom_module_position(mod, position = 2)$ui
function (id) 
{
    ns <- NS(id)
    teal.widgets::standard_layout(output = verbatimTextOutput(ns("text")), 
        encoding = tags$div(selectInput(ns("dataname"), "Choose a dataset", 
            choices = NULL), ui_transform_data(transform_id(id), 
            transformers), teal.widgets::verbatim_popup_ui(ns("rcode"), 
            "Show R code")))
}
<environment: 0x000001e800f38ad0>
> custom_module_position(mod, position = 3)$ui
function (id) 
{
    ns <- NS(id)
    teal.widgets::standard_layout(output = verbatimTextOutput(ns("text")), 
        encoding = tags$div(selectInput(ns("dataname"), "Choose a dataset", 
            choices = NULL), teal.widgets::verbatim_popup_ui(ns("rcode"), 
            "Show R code"), ui_transform_data(transform_id(id), 
            transformers)))
}
<environment: 0x000001e800f38ad0>

@m7pr
Copy link
Contributor

m7pr commented Aug 9, 2024

Hey @vedhav it was hard to propose a suggestion on the text that was not edited in the PR so I allowed myself to make a commit. 799740b with few propositions for changes. We can comment under those changes in the GitHub UI now.

I had an impression that there is too much poetic and unclear statements in the introduction. I just made it more developer-like-documentation specific. Let me know your thoughts on the changes. No hard feelings if you still think we can improve this.

@vedhav
Copy link
Contributor Author

vedhav commented Aug 9, 2024

@m7pr I think we will have a hard time maintaining this wrapper function:

  1. It only works when the teal.widgets::standard_layout is used with encoding. So, this does not work on teal.modules.general::tm_variable_browser()
  2. Even if we handle all the UI cases, we create a reverse dependency of managing the custom_module_position() function when some UI structure of the downstream packages.
  3. If there are custom modules created by people, there is a chance that our wrapper might not work so it's hard to recommend it to people.
  4. With all these cons, I do not like the fact that we have an exported wrapper when there is a way to position UI in any module if they know what to do.

I think it will be a good to circle back on this topic when we implement the teal.transform wrappers for data extracts. It might become useful to create some utility around that time. I think now we are early.

@m7pr
Copy link
Contributor

m7pr commented Aug 9, 2024

@vedhav totally agree, especially that the solution is based on encoding = name of the argument. Name does not need to be specified :) However this is a good starting point - we at least had a chance to figure out how to manage functions with body and as.call so that ui changes in the future might be more flexible. We can delete the wrapper for now.

Any comments to the content changes of the vignette?

@vedhav
Copy link
Contributor Author

vedhav commented Aug 9, 2024

I like the content changes :)

@m7pr
Copy link
Contributor

m7pr commented Aug 9, 2024

Hey I do approve but I'd like @donyunardi to review again, as he said he reviewed it previously. Also @donyunardi it is a chance for you to get up to date with the current state of the solution

Copy link
Contributor

@donyunardi donyunardi left a comment

Choose a reason for hiding this comment

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

Very nice docs and really capture the new feature nicely.
Just small comments here and there.

@vedhav vedhav merged commit ba83d53 into 669_insertUI@main Aug 12, 2024
1 check passed
@vedhav vedhav deleted the extend-teal-transform-vignette@669_insertUI@main branch August 12, 2024 07:40
@github-actions github-actions bot locked and limited conversation to collaborators Aug 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants