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.

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

Use CSS variables and static html dependency #36

Merged
merged 31 commits into from
Jan 21, 2024
Merged

Conversation

gadenbuie
Copy link
Owner

@gadenbuie gadenbuie commented Jan 20, 2024

This PR rewrites countdown's CSS to use CSS variables rather than the previous whisker template. This has a number of advantages, but results in a large breaking change.

  1. We now can use html_dependency_countdown(), a static html dependency. Previously the css was a template and the dependency was re-written dynamically with each render, which causes problems and obviously can't be used for Quarto. This fixes the issue mentioned here: quarto shortcode #27 (comment)

  2. The breaking change is that the first countdown() no longer styles the other timers. In retrospect this is a bug masquerading as a feature 😄 Now, users need to separately call style_countdown() to globally style the countdown timers.

  3. I've kept all of the same arguments in countdown() but they all default to NULL now. style_countdown() is documented in the same page so that the arguments and functions are collocated.

Fixes #28
Fixes #34
Incidentally fixes #30

@gadenbuie gadenbuie marked this pull request as ready for review January 20, 2024 14:34
Copy link
Collaborator

@coatless coatless left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Only a minor note on the style_countdown() function.

Please merge when ready. I'll spin up a second PR for the re-org. (Or, if you'd like I can wait until the #37 is complete.)

R/countdown.R Outdated
#' should be applied. The default is `:root` for global styles, but you can
#' also provide a custom class name to create styles for a particular class.
#' @export
style_countdown <- function(
Copy link
Collaborator

Choose a reason for hiding this comment

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

To match with other function names in the package, maybe go:

countdown_styler()

instead of:

style_countdown()
Names of functions inside the development version of `countdown`

Copy link
Owner Author

@gadenbuie gadenbuie Jan 21, 2024

Choose a reason for hiding this comment

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

Thanks for calling this out. style_{thing} is a pattern I tend to follow across my projects, even though it breaks the usual prefixing rules. In Shiny/html land, I tend to prefer a verb-first approach to separate between functions that create UI and functions for interactivity.

For example, looking at this list I'd prefer update_countdown() and style_countdown(), since in Shiny we group update_ functions and in my packages I use style_ for prefixes.

The problem -- and the reason I didn't use update_countdown() when I wrote it -- is countdown_action(). I can't think of a good name for that function. The other best option I've come up with is trigger_countdown(), which I don't like enough to use. So, IIRC, I went with countdown_update() for internal consistency.

So, on the whole, I agree and think countdown_style() is probably the best option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gadenbuie We can also use this as an opportunity to rename functions if you wish. Redirects can be setup for backward compatibility with lifecycle.

}
if (!grepl("^[a-zA-Z]", id)) {
stop_because("Must start with a letter")
make_countdown_css_vars <- function(..., .list = list()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a really elegant and concise way of setting the CSS variables.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks! Sometimes it pays to be lazy (and to be consistent in naming things) 😉

@coatless
Copy link
Collaborator

I'm going to go ahead and merge this PR.

I'll rebase #37 and, then, I'll start the re-org.

@coatless coatless merged commit 260812e into main Jan 21, 2024
5 checks passed
@gadenbuie gadenbuie deleted the css-vars-rewrite branch January 21, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants