Skip to content

Fixes the long-standing shiny modules problem#127

Merged
andrie merged 20 commits into
mainfrom
modules
Dec 10, 2025
Merged

Fixes the long-standing shiny modules problem#127
andrie merged 20 commits into
mainfrom
modules

Conversation

@andrie

@andrie andrie commented Dec 7, 2025

Copy link
Copy Markdown
Member

Fixes #100

@andrie andrie assigned schloerke and unassigned schloerke Dec 7, 2025
@andrie

andrie commented Dec 7, 2025

Copy link
Copy Markdown
Member Author

@schloerke , we discussed this a long time ago.
It would be great if you can review.
I'd like to submit to CRAN this week.

@yogat3ch

yogat3ch commented Dec 7, 2025

Copy link
Copy Markdown

Sounds like solving this took a deep dive into the way shiny modules work to find the incompatibility. Thank you @andrie & @schloerke for sticking with this one long enough to resolve it!

@schloerke schloerke left a comment

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.

Sorry for the delay. I'd rather have it take a different approach. See below.

Q: Are there reasons to not rip the band-aid here?

Comment thread R/zzz.R
Comment on lines +8 to +15
packageStartupMessage(
# rlang::inform("This message will appear only once per session.", .frequency = "once", .frequency_id = "sortable"),
rlang::inform(
cli::cli_text("To use sortable with shiny modules, run {.run sortable::enable_modules()} to opt into the new standard."),
.frequency = "once",
.frequency_id = "sortable"
)
)

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.

I believe this constant messaging and inconsistent behavior is harder to understand than a big "BREAKING CHANGE, all your sortable DOM IDs have been updated. <previous_format> is now <new_format>. You only need to worry if you have custom css. Thank you for your patience as we have learned from our mistakes." in the NEWS.md

This would remove the startup message, remove the sortable_env, remove the vignette for inconsistent behavior.

I'd rather lean towards "at version X, the behavior changed to something simple and expected".

@andrie andrie merged commit 5505fa7 into main Dec 10, 2025
1 check passed
@andrie andrie deleted the modules branch December 10, 2025 21:43
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.

Sortable and shiny modules are currently incompatible

3 participants