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

Fix early dev app #132

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Fix early dev app #132

merged 1 commit into from
Jan 30, 2024

Conversation

vedhav
Copy link
Contributor

@vedhav vedhav commented Jan 30, 2024

Fixes the error reported here

Changes:

  1. Uses proper parent-key mapping in the join_keys so the filter panel provides the valid data to the modules.
  2. Standardise the library calls across the apps.
300445426-5aeca3ac-8f1b-45c3-8693-2f218a7e5fb5

@vedhav vedhav added bug Something isn't working core labels Jan 30, 2024
@vedhav vedhav requested a review from m7pr January 30, 2024 03:20
Copy link
Contributor

@m7pr m7pr left a comment

Choose a reason for hiding this comment

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

Thanks @vedhav

@vedhav vedhav merged commit 74c770d into dev Jan 30, 2024
@vedhav vedhav deleted the fix-early-dev-app branch January 30, 2024 09:33
@vedhav
Copy link
Contributor Author

vedhav commented Jan 30, 2024

Should be re-deployed in ~10 mins to test. Can be tested in the same link: https://genentech.shinyapps.io/nest_early-dev_dev

Comment on lines +14 to +15
# optional libraries
library(sparkline)
Copy link
Contributor

Choose a reason for hiding this comment

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

apologies that I jumped into the already closed PR
may I ask: why this particular change?
AFAIK sparkline is a supplementary module package - it's not required for data loading part (as opposed to dplyr, scda etc. which are correct here in this context). Therefore I would expect it to be at the beggining of the app.R script and not inside within().

(same for safety/app.R)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a misconception about this package. I remember that some app had error/warning when this library was not loaded. I think we can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that loading this library would improve the variable browser module.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right. It's needed for the variable browser. I got the same understanding. But the position is quite unusual now because you moved the library into the teal_data(...) %>% within(<here>) and usually we have modules libs load outside of this at the top of the script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants