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.

Sign up for GitHub

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

edit card name #172

Merged
merged 14 commits into from
Nov 25, 2022
Merged

edit card name #172

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
dcc2242
edit card name
mhallal1 Nov 22, 2022
84fa80b
Merge dcc224255d3ff7424c70ad4d58b2e51cd16b9f0b into 927e18afc1d6668d1…
mhallal1 Nov 22, 2022
394d142
[skip actions] Restyle files
github-actions[bot] Nov 22, 2022
039aea3
update
mhallal1 Nov 22, 2022
426fc9c
Merge remote-tracking branch 'origin/94_editable_card_names@main' int…
mhallal1 Nov 22, 2022
96f3ad0
update
mhallal1 Nov 22, 2022
26e3c1d
docs
mhallal1 Nov 23, 2022
7535fc7
Merge branch 'main' into 94_editable_card_names@main
mhallal1 Nov 23, 2022
e810df0
Apply suggestions from code review
mhallal1 Nov 23, 2022
51fc449
review
mhallal1 Nov 23, 2022
9a7bafa
Merge remote-tracking branch 'origin/94_editable_card_names@main' int…
mhallal1 Nov 23, 2022
04545a5
docs
mhallal1 Nov 24, 2022
cfe0dfd
NEWS
mhallal1 Nov 24, 2022
fc0d66f
docs
mhallal1 Nov 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions R/AddCardModule.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ add_card_button_ui <- function(id) {
add_card_button_srv <- function(id, reporter, card_fun) {
checkmate::assert_function(card_fun)
checkmate::assert_class(reporter, "Reporter")
checkmate::assert_subset(names(formals(card_fun)), c("card", "comment"), empty.ok = TRUE)
checkmate::assert_subset(names(formals(card_fun)), c("card", "label", "comment"), empty.ok = TRUE)

shiny::moduleServer(
id,
Expand All @@ -89,7 +89,13 @@ add_card_button_srv <- function(id, reporter, card_fun) {
shiny::modalDialog(
easyClose = TRUE,
shiny::tags$h3("Add a Card to the Report"),
shiny::tags$hr(),
shiny::textInput(
mhallal1 marked this conversation as resolved.
Show resolved Hide resolved
ns("label"),
"Card Name",
value = "",
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like the modules should be able to give a default to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this requires passing label as an argument to add_card_button_srv and simple_reporter_srv.

Copy link
Contributor

@Polkas Polkas Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Now I think it could be a good move as we are not hiding some value internally. The label argument can by "" by default.

Copy link
Contributor

@Polkas Polkas Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Another way can be to not have any default for label then to have empty name if not specified.when card is added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup that's definitely a better way of adding it - though we'd still need to pass the label into the module args - might be best to not do that right now though and wait until teal is reconfigured to make that not needed...

Copy link
Contributor

Choose a reason for hiding this comment

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

Another way can be to not have any default for label then to have empty name if not specified.when card is added.

Or to have the name the module developer has set for it (rather than the app developer).

Copy link
Contributor Author

@mhallal1 mhallal1 Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

so the latest update is:

  1. the module developer can set a default card label in the card function (it can be unnamed also)
  2. the module user can change the card label with the UI input
  3. No support for passing the module name by default to the reporter cards till the teal refactor is accomplished as it requires passing the label argument to the server function of all modules.

placeholder = "Add the card title here",
width = "100%"
),
shiny::textAreaInput(
ns("comment"),
"Comment",
Expand Down Expand Up @@ -140,14 +146,18 @@ add_card_button_srv <- function(id, reporter, card_fun) {
card_fun_args_nams <- names(formals(card_fun))
has_card_arg <- "card" %in% card_fun_args_nams
has_comment_arg <- "comment" %in% card_fun_args_nams

has_label_arg <- "label" %in% card_fun_args_nams
mhallal1 marked this conversation as resolved.
Show resolved Hide resolved

arg_list <- list()

if (has_comment_arg) {
arg_list <- c(arg_list, list(comment = input$comment))
}

if (has_label_arg && length(input$label) > 0 && input$label != "") {
arg_list <- c(arg_list, list(label = input$label))
}

if (has_card_arg) {
# The default_card is defined here because formals() returns a pairedlist object
# of formal parameter names and their default values. The values are missing
Expand Down Expand Up @@ -180,6 +190,13 @@ add_card_button_srv <- function(id, reporter, card_fun) {
card$append_text("Comment", "header3")
card$append_text(input$comment)
}

if (!has_label_arg && input$label == "" && length(card$get_name()) == 0) {
card$set_name("")
} else if (!has_label_arg && length(input$label) > 0 && input$label != "") {
card$set_name(input$label)
}

reporter$append_cards(list(card))
shiny::showNotification(sprintf("The card added successfully."), type = "message")
shiny::removeModal()
Expand Down
2 changes: 1 addition & 1 deletion R/SimpleReporter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ simple_reporter_ui <- function(id) {
#' @param id `character(1)` this `shiny` module's id.
#' @param reporter [`Reporter`] instance.
#' @param card_fun `function` which returns a [`ReportCard`] instance,
#' the function have at`card`argument and optional `comment`.
#' the function has a `card` argument and optional `comment` and `label` arguments.
#' @return `shiny::moduleServer`
#' @export
simple_reporter_srv <- function(id, reporter, card_fun) {
Expand Down
2 changes: 1 addition & 1 deletion man/simple_reporter_srv.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.