Skip to content

Conversation

@averissimo
Copy link
Contributor

@averissimo averissimo commented Oct 24, 2025

Pull Request

Companion PR:

Note to reviewer

ℹ️ Please check each example app of the affected module

Changes description

  • Adds bslib::accordion wrapper to plot settings that were missing
  • Check other modules / encoding if there are any that have the same problem

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2025

Unit Tests Summary

  1 files   71 suites   41s ⏱️
734 tests 272 ✅ 462 💤 0 ❌
877 runs  411 ✅ 466 💤 0 ❌

Results for commit d7778d3.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2025

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
examples 💔 $2.35$ $+4.60$ $0$ $-115$ $0$ $0$
tm_t_glm_counts 💔 $0.50$ $+23.95$ $+3$ $-2$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
tm_t_glm_counts 💔 $0.33$ $+11.76$ e2e_tm_t_glm_counts_Module_initializes_in_teal_without_errors_and_produces_table_output.
tm_t_glm_counts 💔 $0.17$ $+12.19$ e2e_tm_t_glm_counts_Selecting_arm_var_changes_the_table_and_does_not_throw_validation_errors.

Results for commit 14c9c6e

♻️ This comment has been updated with latest results.

@osenan osenan assigned osenan and unassigned osenan Oct 27, 2025
Copy link

@osenan osenan left a comment

Choose a reason for hiding this comment

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

Changes look ok. I will review the modules in teal apps to verify they look ok.

@m7pr m7pr requested a review from Copilot October 27, 2025 14:07
Copy link
Contributor

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.

Pull Request Overview

This PR fixes a UI structure issue where plot and table settings panels were missing proper bslib::accordion wrappers. The changes ensure consistent accordion panel structure across multiple modules by wrapping bslib::accordion_panel elements with parent bslib::accordion containers and moving the open = TRUE parameter from the panel to the accordion level.

  • Adds bslib::accordion wrapper around existing bslib::accordion_panel elements
  • Moves open = TRUE parameter from panel level to accordion level
  • Restructures one table settings component to use tagList wrapper

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
R/tm_t_tte.R Wraps "Additional table settings" panel with accordion and fixes formatting
R/tm_t_glm_counts.R Restructures table settings with tagList and accordion wrapper
R/tm_t_events.R Wraps "Additional table settings" panel with accordion
R/tm_t_binary_outcome.R Wraps "Additional table settings" panel with accordion
R/tm_g_pp_vitals.R Wraps "Plot settings" panel with accordion
R/tm_g_pp_therapy.R Wraps "Plot settings" panel with accordion
R/tm_g_pp_patient_timeline.R Wraps "Plot settings" panel with accordion
R/tm_g_pp_adverse_events.R Wraps "Plot settings" panel with accordion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@osenan osenan left a comment

Choose a reason for hiding this comment

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

I've checked all modules in teal.gallery apps except tm_t_glm_counts that is not used.
With the changes, is looking much better.

When all changes are done, we need to update the teal.gallery dependencies

@averissimo averissimo merged commit 040f0e0 into main Oct 29, 2025
26 checks passed
@averissimo averissimo deleted the 937-plot_settings branch October 29, 2025 13:28
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Question]: Outlier module: plot setting is unavailable despite showing title?

4 participants