Skip to content

1298 enhancement/cleanup code quality #1320

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

Open
wangzhengdna-lang wants to merge 8 commits into
base: main
Choose a base branch
from
Open

1298 enhancement/cleanup code quality #1320

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0864436
chore: clean up debug statements, deprecated attributes, and improve …
May 26, 2026
fd89731
chore: extract inline hex colors to shared R constants and SCSS varia…
May 26, 2026
6b8de75
fix: address review comments — glue interpolation, accessibility, and…
May 27, 2026
a5b78fe
style: fix line-length lints from glue and paste0 refactoring (#1298)
May 27, 2026
80d9d74
chore: add actionButton and SCSS to WORDLIST
Shaakon35 May 28, 2026
eb45ab8
Merge branch 'main' into 1298-enhancement/cleanup-code-quality
Shaakon35 May 29, 2026
fba2909
fix: correct .gitignore corruption and use tooltip instead of label f…
wangzheng Jun 4, 2026
3511923
fix: wrap pkcg02 example in \donttest{} to avoid Windows CI timeout (…
wangzheng Jun 4, 2026
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
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Package: aNCA
Title: (Pre-)Clinical NCA in a Dynamic Shiny App
Version: 0.1.0.9173
Version: 0.1.0.9174
Authors@R: c(
person("Ercan", "Suekuer", email = "ercan.suekuer@roche.com", role = "aut",
comment = c(ORCID = "0009-0001-1626-1526")),
Expand Down
10 changes: 10 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# aNCA (development version)

## Code Quality

* Removed debug `print()` statements from TLG module and replaced with proper logging (#1298)
* Removed unused `require(magrittr)` and `require(stats)` calls from `app.R` (#1298)
* Replaced deprecated HTML `align` attributes with inline CSS in TLG module (#1298)
* Standardized button CSS classes: removed redundant `btn` prefix where duplicate with `btn-*` (#1298)
* Added `aria-label` attributes to 15 icon-only buttons for accessibility (#1298)
* Removed redundant `disabled = FALSE` default from units table actionButton (#1298)
* Extracted inline hex colors to shared `colors.R` constants and SCSS variables (#1298)

## Features

### Settings & Configuration
Expand Down
2 changes: 0 additions & 2 deletions inst/shiny/app.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ require(dplyr)
require(htmlwidgets)
require(logger)
require(formatters)
require(magrittr)
require(plotly)
require(purrr)
require(reactable)
Expand All @@ -15,7 +14,6 @@ require(shinycssloaders)
require(shinyjs)
require(shinyjqui)
require(shinyWidgets)
require(stats)

@Shaakon35 Shaakon35 May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

require(stats) removal — likely safe but worth verifying

Removing require(magrittr) is clearly correct (%>% is re-exported by dplyr). Removing require(stats) is almost certainly fine since stats is a base package that's always loaded in R sessions, but worth a quick grep -r "stats::" inst/shiny/ to confirm no module code relies on the namespace being explicitly attached.

@wangzhengdna-lang wangzhengdna-lang May 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good suggestion, grep -r "stats::" inst/shiny/ returns zero results.


require(tidyr)
require(tools)
Expand Down
26 changes: 26 additions & 0 deletions inst/shiny/functions/colors.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Color constants — keep in sync with inst/shiny/www/styles/modules/_colors.scss

@Shaakon35 Shaakon35 May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Color constants duplicate and diverge from existing SCSS variables

The existing _colors.scss already defines a palette ($anca-blue: #007bc2, $grey-1 through $grey-5). The new constants here introduce a parallel set that doesn't reference or align with those:

  • ACCENT_BLUE is #0d6efd (Bootstrap blue) vs $anca-blue: #007bc2 — different blues, but naming doesn't distinguish intent.
  • BG_GREY_LIGHT is #eeeeee vs $grey-5: #dadada — similar purpose, different values.

The sync comment on line 1 is good, but there's no mechanism to enforce it. A future developer changing the SCSS variable won't know to update the R constant (and vice versa).

Suggestion: Add a matching comment in _colors.scss pointing back to this file (bidirectional sync note). Also consider whether some of these should reuse existing SCSS variable values rather than introducing new ones.

@wangzhengdna-lang wangzhengdna-lang May 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the review. Now added bidirectional sync comment in _colors.scss pointing back to colors.R (commit a5b78fe). The new variables expand the shared palette — they mirror the R constants and will be consumed by future CSS rules. For now both files serve as the single source of truth for their respective domains (SCSS for CSS rules, R for inline styles).

# See https://github.com/pharmaverse/aNCA/issues/1298

# Backgrounds
BG_GREY_LIGHT <- "#eeeeee"
BG_GREY_HEADER <- "#e9e9e9"
BG_HIGHLIGHT_BLUE <- "#CCE5FF"
BG_BLUE_LIGHT <- "#e6f2ff"
ANCA_WHITE <- "#ffffff"

# Text
TEXT_MUTED <- "#999999"
TEXT_DIM <- "#888888"
TEXT_SUBTLE <- "#777777"
TEXT_SECONDARY <- "#666666"
TEXT_HEADING <- "#555555"

# Borders
BORDER_LIGHT <- "#dddddd"
BORDER_MUTED <- "#cccccc"
BORDER_PANEL <- "#dee2e6"

# Accent
ACCENT_BLUE <- "#0d6efd"
ACCENT_ORANGE <- "#ffa62d"
ACCENT_DANGER <- "#dc3545"
7 changes: 4 additions & 3 deletions inst/shiny/functions/utils-exclusions.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ EXCL_COLOR_PARAM <- "#FFF3CD" # yellow — parameter exclusion
style = "display:flex; align-items:center; gap:6px;",
div(style = paste0(
"width:14px; height:14px; background:", color,
"; border:1px solid #ddd;"
"; border:1px solid ", BORDER_LIGHT, ";"
)),
span(label, style = "font-size:0.9em;")
)
Expand Down Expand Up @@ -167,7 +167,8 @@ EXCL_COLOR_PARAM <- "#FFF3CD" # yellow — parameter exclusion
ns(value),
label = NULL,
icon = shiny::icon("times"),
class = "btn btn-link btn-sm",
`aria-label` = "Remove exclusion",
class = "btn-link btn-sm",
style = "padding:2px 6px;"
)
)
Expand Down Expand Up @@ -196,7 +197,7 @@ EXCL_COLOR_PARAM <- "#FFF3CD" # yellow — parameter exclusion
pagination = FALSE,
defaultPageSize = nrow(df),
theme = reactable::reactableTheme(
headerStyle = list(background = "#e9e9e9")
headerStyle = list(background = BG_GREY_HEADER)
),
columns = cols
)
Expand Down
6 changes: 3 additions & 3 deletions inst/shiny/modules/tab_about.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ tab_about_server <- function(id) {
}
)
tags$blockquote(
style = "border-left: 3px solid #ccc; padding-left: 1em; color: #555;",
style = paste0("border-left: 3px solid ", BORDER_MUTED, "; padding-left: 1em; color: ", TEXT_HEADING, ";"),

@Shaakon35 Shaakon35 May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

paste0() for inline styles reduces readability

The original one-liner was easy to scan. The replacement fragments the string across multiple paste0() concatenations:

# Before
style = "border-left: 3px solid #ccc; padding-left: 1em; color: #555;"

# After
style = paste0("border-left: 3px solid ", BORDER_MUTED, "; padding-left: 1em; color: ", TEXT_HEADING, ";")

Since glue is already in DESCRIPTION Imports, consider using it for cleaner interpolation:

style = glue("border-left: 3px solid {BORDER_MUTED}; padding-left: 1em; color: {TEXT_HEADING};")

This applies to all similar paste0() style interpolations in this PR (this file, saved_outputs.R, zip.R, etc.).

@wangzhengdna-lang wangzhengdna-lang May 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Switched all color-related style interpolations from paste0() to glue::glue() across 8 files. glue was already in DESCRIPTION Imports so no dependency change. The nested paste()/paste0() block in zip.R is now a single glue::glue() call.

tags$p(cit_text)
)
})
Expand All @@ -163,7 +163,7 @@ tab_about_server <- function(id) {
cit <- utils::citation("PKNCA")
cit_text <- paste(format(cit, style = "text"), collapse = "\n\n")
tags$blockquote(
style = "border-left: 3px solid #ccc; padding-left: 1em; color: #555;",
style = paste0("border-left: 3px solid ", BORDER_MUTED, "; padding-left: 1em; color: ", TEXT_HEADING, ";"),
tags$p(cit_text)
)
})
Expand Down Expand Up @@ -255,7 +255,7 @@ tab_about_server <- function(id) {
style = "margin-bottom: 0.3em;",
tags$a(href = url, target = "_blank", label),
tags$span(
style = "color: #999; font-size: 0.85em; margin-left: 0.5em;",
style = paste0("color: ", TEXT_MUTED, "; font-size: 0.85em; margin-left: 0.5em;"),
url
)
)
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_data/data_mapping.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ data_mapping_server <- function(id, adnca_data, imported_mapping, trigger) {
if (dup_data[index, ".dup_group"] %% 2 == 0) {
list(background = "white")
} else {
list(background = "#e6f2ff")
list(background = BG_BLUE_LIGHT)
}
},
selection = "multiple",
Expand Down
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_data/data_upload.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,8 @@ data_upload_server <- function(id) {
defaultPageSize = 10,
theme = reactableTheme(
rowSelectedStyle = list(
backgroundColor = "#CCE5FF",
boxShadow = "inset 2px 0 0 0 #0d6efd"
backgroundColor = BG_HIGHLIGHT_BLUE,
boxShadow = paste0("inset 2px 0 0 0 ", ACCENT_BLUE)
)
),
columns = list(
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_explore.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ tab_explore_server <- function(id, pknca_data, extra_group_vars) {
footer = tagList(
modalButton("Cancel"),
actionButton(ns("confirm_add_to_exports"), "Save",
class = "btn btn-primary")
class = "btn-primary")
),
size = "s",
easyClose = TRUE
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_explore/plot_sidebar.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ plot_sidebar_ui <- function(id, is_mean_plot = FALSE, extra_ui = NULL) {
ns("add_to_exports"),
label = "Add to Exports",
icon = icon("plus"),
class = "btn btn-primary btn-sm",
class = "btn-primary btn-sm",
width = "100%"
),
extra_ui,
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_explore/qc_plot.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pk_dose_qc_plot_ui <- function(id, extra_ui = NULL) {
ns("add_to_exports"),
label = "Add to Exports",
icon = icon("plus"),
class = "btn btn-primary btn-sm",
class = "btn-primary btn-sm",
width = "100%"
),
extra_ui,
Expand Down
13 changes: 7 additions & 6 deletions inst/shiny/modules/tab_explore/saved_outputs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ saved_outputs_ui <- function(id) {
ns("view_exports"),
label = "View Exports",
icon = icon("eye"),
class = "btn btn-primary btn-sm",
class = "btn-primary btn-sm",
width = "100%"
)
}
Expand Down Expand Up @@ -168,7 +168,7 @@ saved_outputs_server <- function(id, saved_plots_metadata, get_plot_obj,
"No plots saved yet. Use ",
tags$strong("Add to Exports"),
" to save plots.",
style = "color: #888; font-style: italic; margin: 16px 0;"
style = paste0("color: ", TEXT_DIM, "; font-style: italic; margin: 16px 0;")
))
}

Expand All @@ -187,8 +187,8 @@ saved_outputs_server <- function(id, saved_plots_metadata, get_plot_obj,

tags$div(
style = paste0(
"border: 1px solid #dee2e6; border-radius: 6px; ",
"padding: 12px; margin-bottom: 16px; background: #fff;"
"border: 1px solid ", BORDER_PANEL, "; border-radius: 6px; ",
"padding: 12px; margin-bottom: 16px; background: ", ANCA_WHITE, ";"
),
tags$div(
style = paste0(
Expand All @@ -199,16 +199,17 @@ saved_outputs_server <- function(id, saved_plots_metadata, get_plot_obj,
tags$strong(plot_name, style = "font-size: 1.1em;"),
tags$span(
paste0(" \u2014 ", plot_type, " \u00b7 ", plot_ts),
style = "color: #666; font-size: 0.9em;"
style = paste0("color: ", TEXT_SECONDARY, "; font-size: 0.9em;")
)
),
tags$a(
href = "#",
onclick = paste0(remove_js, "return false;"),
style = paste0(
"color: #dc3545; font-size: 1.2em; ",
"color: ", ACCENT_DANGER, "; font-size: 1.2em; ",
"text-decoration: none;"
),
`aria-label` = "Remove from exports",
title = "Remove from exports",
icon("times")
)
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_nca.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ tab_nca_ui <- function(id) {
inputId = ns("run_nca"),
label = "Run NCA",
icon = icon("play"),
class = "btn btn-primary",
class = "btn-primary",
width = "100%"
),
nca_setup_ui(ns("nca_setup")),
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_nca/excretion_analysis.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ excretion_help_ui <- function() {
)
),
style = "unite",
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",

@Shaakon35 Shaakon35 May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

aria-label on dropdown() — verify it reaches the DOM

The aria-label is passed to shinyWidgets::dropdown(), not directly to an HTML <button> element. This works only if dropdown() forwards ... args to the trigger button. If it doesn't, the aria-label won't appear in the rendered HTML.

Please verify in the browser devtools (as described in the test instructions) that the aria-label="Help" attribute actually appears on the rendered <button> element for at least one of these 12 dropdown instances.

@wangzhengdna-lang wangzhengdna-lang May 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's right, shinyWidgets::dropdown() passes ... to the inner content div . Verified this by reading the dropdown() source. Replaced aria-label = "Help" with label = "Help" on all 12 dropdown instances. The label parameter is the native way to add accessible text to the dropdown trigger button.

status = "primary"
)
}
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_nca/nca_results.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ nca_results_ui <- function(id) {
span("MISSING", style = "font-size:0.9em;")
),
div(style = "display:flex; align-items:center; gap:6px;",
div(style = "width:14px; height:14px; background:#ffffff; border:1px solid #ddd;"),
div(style = paste0("width:14px; height:14px; background:", ANCA_WHITE, "; border:1px solid ", BORDER_LIGHT, ";")),
span("ACCEPTED", style = "font-size:0.9em;")
)
),
Expand Down
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_nca/non_nca_ratios.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ non_nca_ratio_server <- function(id, data, grouping_vars) {
),
div(
class = "ratio-remove-btn-wrapper",
actionButton(ns(paste0("remove_", count)), "", icon = icon("trash-alt"))
actionButton(ns(paste0("remove_", count)), "", `aria-label` = "Remove ratio pair", icon = icon("trash-alt"))
)
)

Expand Down Expand Up @@ -243,7 +243,7 @@ non_nca_ratio_help_ui <- function(title) {
),
style = "unite",
placement = "left",
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary"
)
}
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_nca/setup/data_imputation.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ data_imputation_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary",
width = "500px"
)
Expand Down Expand Up @@ -123,7 +123,7 @@ data_imputation_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary",
width = "500px"
)
Expand Down
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_nca/setup/general_exclusions.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ general_exclusions_ui <- function(id) {
actionButton(
ns("add_exclusion_reason"),
label = "Add",
class = "btn btn-primary btn-sm"
class = "btn-primary btn-sm"
),
# Help button (dropdown)
dropdown(
Expand Down Expand Up @@ -78,7 +78,7 @@ general_exclusions_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary"
)
),
Expand Down
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_nca/setup/manual_slopes_table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ manual_slopes_table_server <- function(
borderless = TRUE,
theme = reactableTheme(
rowSelectedStyle = list(
backgroundColor = "#eee",
boxShadow = "inset 2px 0 0 0 #ffa62d"
backgroundColor = BG_GREY_LIGHT,
boxShadow = paste0("inset 2px 0 0 0 ", ACCENT_ORANGE)
)
)
)
Expand Down
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_nca/setup/parameter_exclusions.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ parameter_exclusions_ui <- function(id) {
actionButton(
ns("add_exclusion"),
label = "Add",
class = "btn btn-primary btn-sm"
class = "btn-primary btn-sm"
),
dropdown(
div(
Expand All @@ -45,7 +45,7 @@ parameter_exclusions_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary"
)
),
Expand Down
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_nca/setup/parameter_selection.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ parameter_selection_ui <- function(id,
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary",
width = "500px"
),
Expand Down Expand Up @@ -428,7 +428,7 @@ parameter_selection_server <- function(id, processed_pknca_data, parameter_overr
href = func_url,
target = "_blank",
style = paste0(
"color: #0d6efd;",
"color: ", ACCENT_BLUE, ";",
"text-decoration: underline;"
),
value
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_nca/setup/ratio_calculations_table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ ratios_table_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary",
width = "500px"
)
Expand Down
8 changes: 4 additions & 4 deletions inst/shiny/modules/tab_nca/setup/settings.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ partial_intervals_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary",
width = "600px"
)
Expand Down Expand Up @@ -186,7 +186,7 @@ settings_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary",
width = "400px"
)
Expand Down Expand Up @@ -403,8 +403,8 @@ settings_server <- function(id, data, adnca_data, settings_override) {
borderless = TRUE,
theme = reactableTheme(
rowSelectedStyle = list(
backgroundColor = "#eee",
boxShadow = "inset 2px 0 0 0 #ffa62d"
backgroundColor = BG_GREY_LIGHT,
boxShadow = paste0("inset 2px 0 0 0 ", ACCENT_ORANGE)
)
)
)
Expand Down
2 changes: 1 addition & 1 deletion inst/shiny/modules/tab_nca/setup/slope_selector.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ slope_selector_ui <- function(id) {
),
style = "unite",
right = TRUE,
icon = icon("question"),
icon = icon("question"), `aria-label` = "Help",
status = "primary",
width = "600px"
),
Expand Down
Loading
Loading