-
-
Notifications
You must be signed in to change notification settings - Fork 12
picks #247
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
base: dev
Are you sure you want to change the base?
Conversation
|
✅ All contributors have signed the CLA |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Related issues in the milestone:
See related PRs:
Installation