Skip to content

Conversation

@gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Oct 30, 2025

Related issues in the milestone:

See related PRs:

Installation

remotes::install_github("insightsengineering/teal.modules.general@redesign_extraction@main")

@gogonzo gogonzo added the core label Oct 30, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@gogonzo
Copy link
Contributor Author

gogonzo commented Oct 30, 2025

I have read the CLA Document and I hereby sign the CLA

@gogonzo gogonzo changed the title Picks picks Oct 31, 2025
@gogonzo gogonzo mentioned this pull request Oct 31, 2025
Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

It looks like we get default columns with 70 less lines of code. Nice.

Why do most picks() on the app have values()? I understand why we want to specify a value for filtering on a teal_transform_filter() or why tm_g_distribution() some parameters use values() while others don't: those with values is to specify a given value as reference, response or for plot customization.
But on other modules, like tm_g_scatterplot() or tm_t_crosstable(), values() is used even for x and y where I don't expect specific values to be needed:

From the description of picks on insightsengineering/teal.transform#270 it is not apparent that we need to specify it for merging and keeping those columns.
Answer: The values() is added to be able to filter dynamically on each variable. It is also reset if the selection changes.

tm_g_scatterplotmatrix(
label = "Scatterplot Matrix",
variables = adsl_extracted_multi
variables = list(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we have list(picks()) instead of directly picks(). What does this add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have list(picks()) instead of directly picks(). What does this add?

scatterplotmatrix have unknown number of "arguments" (variables), having a list here allows to specify variables from multiple datasets.


those with values is to specify a given value as reference, response or for plot customization.

Yup, filter on an dimension-variable (x, y, color, size etc) is set by default in the modules. I think it is a nice default when app-user can filter axis after selecting a variable. On friday's meeting we discovered that picks-arguments which allow multiple-variables, it is wrong to set a values()

From the description of picks on insightsengineering/teal.transform#270 it is not apparent that we need to specify it for merging and keeping those columns

Yup, I should link merge_module in the picks documentation

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