Skip to content

Conversation

@samussiah
Copy link
Contributor

No description provided.

@jwildfire jwildfire self-requested a review March 8, 2023 14:38
Copy link
Contributor

@jwildfire jwildfire left a comment

Choose a reason for hiding this comment

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

Left a few questions for you @samussiah. In short, I think I'd scale this back a little bit for now. We can think about a more complex implementation later.

Also need to update the @value in the roxygen header to describe what is being returned.

Happy to chat if needed.

module_outputs
)
} else {
module_output <- output[["chart-wrap"]] <- chart$functions$server(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is anything actually being returned here for non-shiny charts? I guess in theory the main function could return something, but is this just NULL for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, nothing now... I can imagine returning a filtered/transformed dataset or a population subset.

#' @export

chartsTab <- function(input, output, session, chart, data, mapping){
chartsTab <- function(input, output, session, chart, data, mapping, module_outputs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea that this is all of outputs from the other charts? Is this reactive? Are you using this for anything right now? Wondering if we could trigger an infinite loop with this somehow ...

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'm imagining a module that aggregates output from other modules, each with its own set of inputs.

mapping=current_mapping
)
)
module_outputs <- reactiveValues()
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reactiveValues is actually what's working. It allows passing around a single, updateable object of reactive modules.

chart=chart,
data=filtered_data,
mapping=current_mapping,
module_outputs=module_outputs
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be module_outputs1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted the module_outputs of returned map output.

#Setting tab
callModule(settingsTab, "settings", domains = domainData, metadata=meta, mapping=current_mapping, charts = charts)

return(module_outputs1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to return this from the overall server function ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

)

server <- function(input, output, session) {
module_outputs <- callModule(
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we want to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have the server function return something, I'd think it should be a larger config including meta, mapping etc. For now, I don't think the overall server function is well designed to be used as part of a larger app, so I'd lean towards not implementing this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (3)
  • R/mod_chartsNav.R: Language not supported
  • R/mod_chartsTab.R: Language not supported
  • R/mod_safetyGraphicsServer.R: Language not supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants