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

feat: countdown as a Quarto shortcode extension! #35

Merged
merged 36 commits into from
Feb 5, 2024

Conversation

coatless
Copy link
Collaborator

@coatless coatless commented Jan 18, 2024

Close #27

TODO:

  • Rebase (really a big merge commit)
  • Add _extension
  • Re-write R logic into Lua
    • Setup default options where possible
    • Mirror whisker write for CSS
    • Figure whether an option is present and if not use a default
      • Developed getOption(options, key, default) for check/retrieval/or use default and tryOption() for just check/retrieval
    • Re-work how CSS values are set up
    • Think about options for prismatic()
  • Determine document-level settings
countdown:
   font_size: "3rem"
  • Figuring out asset sharing in the monorepo: either duplicate libraries via a script
.
|- lib/
|- quarto/
|  |- _extension/countdown
|- r/
|- python/
|_ README.md

c.f. quarto-dev/quarto-cli#8353

Non-ported functions:

@coatless coatless changed the title [WIP] [WIP] Port into Quarto shortcode extension Jan 18, 2024
@gadenbuie
Copy link
Owner

gadenbuie commented Jan 19, 2024

I don't mean to backseat drive on this, but are you planning on adding (or leaving room for) a python port as well? I could get behind that idea!

But if not, then we might not need to move everything around quite as much and we could let the R package deps in inst/countdown be the source of truth. I'm okay with needing to copy from inst/countdown into _extensions/countdown — since quarto add pulls from main by default it's reasonable to want to be intentional about what's being released into the quarto extension.

@coatless
Copy link
Collaborator Author

@gadenbuie yup; I'm aiming for a py port. If anything, I think it'll be a nice py-shiny/py-shinylive example

@gadenbuie gadenbuie marked this pull request as ready for review January 19, 2024 22:51
@gadenbuie gadenbuie marked this pull request as draft January 19, 2024 22:52
@gadenbuie
Copy link
Owner

@coatless I added you as a collaborator in this repo, welcome! Would you like to review #36 before I merge? It prepares the CSS for use in other projects and removes the weird whisker templating. It also sets up the R interface to be closer to what will likely become the Python API. And finally it means you'd have to re-do some of the work you've already done in this PR (sorry! but I think it's worth it to do this now).

@gadenbuie
Copy link
Owner

@coatless In terms of logistics, I think we should merge this restructuring as early as possible, I guess ideally after merging the two PRs I put up today.

I'm okay with main being a little messy for a while (we're protected by the CRAN release). Merging the big re-org will let us collaborate on various areas to move this all forward and will let us submit smaller PRs. Minimally I think we should:

  1. Re-organize the folder structure
  2. Update the docs for installing the R package from GitHub
  3. Have CI working for the R package folder
  4. Everything else can be stubbed or in a semi broken state to be fixed or implemented in subsequent PRs

@coatless
Copy link
Collaborator Author

@gadenbuie thanks for the invite! I'm not too worried about the re-work. By moving away from mustache-like templating, that'll save one dependency for R (whiskers) and Python (chevron).

How would you feel if I split off the restructuring into a separate PR and, then, rebased this PR ontop of the restructured repository?

@gadenbuie
Copy link
Owner

How would you feel if I split off the restructuring into a separate PR and, then, rebased this PR ontop of the restructured repository?

That sounds great!

@coatless
Copy link
Collaborator Author

@gadenbuie we do have a design choice to think of when it comes to the document-level CSS, e.g. countdown_style().

How should the styles be set in document-level settings?

Option A: Top level option countdown with styles nested under styler

countdown:
  styler: 
     font_size: "3rem"

Option B: Only allow for top-level options; may or may not mix with other permanent settings.

countdown:
   font_size: "3rem"

@gadenbuie
Copy link
Owner

gadenbuie commented Jan 22, 2024

Should we allow globally setting options like warn_when and update_every via the YAML frontmatter? Or just style?

I'm of two minds in terms of the YAML structure for the options. On the one hand, with YAML we can be a bit more expressive than I was with the R function options and we could use the list structure to organize options:

countdown:
  position:
    bottom: 0
    right: 0,
  play_sound: false
  warn_when: 0
  update_every: 1
  blink_colon: true
  start_immediately: false
  style:
    font_size: 
    margin:
    padding:
    box_shadow:
    border_width:
    border_radius:
    line_height:
    color:
      initial:
        border:
        background:
        text:
      running:
        border:
        background:
        text:
      finished:
        border:
        background:
        text:
      warning:
        border:
        background:
        text:

Alternatively, given that everything is already flattened in the R function signature, I think I might prefer to just have all options under countdown and to have their names match with the R parameter names:

countdown:
  play_sound: false
  bottom: 0
  right: 0
  top: null
  left: null
  warn_when: 0L
  update_every: 1L
  blink_colon: false
  start_immediately: false
  font_size: null
  margin: null
  padding: null
  box_shadow: null
  border_width: null
  border_radius: null
  line_height: null
  color_border: null
  color_background: null
  color_text: null
  color_running_background: null
  color_running_border: null
  color_running_text: null
  color_finished_background: null
  color_finished_border: null
  color_finished_text: null
  color_warning_background: null
  color_warning_border: null
  color_warning_text: null

@coatless
Copy link
Collaborator Author

coatless commented Jan 23, 2024

Should we allow globally setting options like warn_when and update_every via the YAML frontmatter? Or just style?

My thought here was to make available settings when possible globally to avoid redefining in long-running presentations. But, maybe we just add that as a feature if an issue ticket comes along?

I think I might prefer to just have all options under countdown and to have their names match with the R parameter names

Okay, we'll aim for Option B then to keep consistency across the packages.

Copy link
Owner

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Things are looking great in here! Thanks for addressing those comments, I've added just a couple more and then this will be ready to go!

coatless and others added 3 commits January 29, 2024 02:42
Co-authored-by: Garrick Aden-Buie <[email protected]>
Condense structureCSSCountdown Vars() to match positional options (`key: value;`) and countdown CSS variables (`---countdown-key: value;`)

Add documentation on parseTimeString
…king for it before attempting to retrieve the sound in `playSound()`.
@coatless
Copy link
Collaborator Author

coatless commented Feb 2, 2024

@gadenbuie I re-worked the default arguments so that we're no longer using a table of named entries but just a list of the parameters and removed unneeded functions.

I also added a check in playSound() within the countdown.js to look for whether the data-play-strong variable is a string false value. Otherwise, the library tried to access a URL with: /false

Pre-patched countdown JS attempting to play a `/false` URL

I'd like to try to get this into the repo this weekend. Given our prior discussions on prismatic, what else should we aim for in the PR?

Copy link
Owner

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

This is looking great! Just a few last little things and then feel free to merge when you're ready. Thanks again for your work on this!

quarto/_extensions/countdown/countdown.lua Outdated Show resolved Hide resolved
quarto/_extensions/countdown/countdown.lua Outdated Show resolved Hide resolved
quarto/_extensions/countdown/countdown.lua Show resolved Hide resolved
coatless and others added 2 commits February 2, 2024 19:09
Co-authored-by: Garrick Aden-Buie <[email protected]>
Co-authored-by: Garrick Aden-Buie <[email protected]>
@coatless
Copy link
Collaborator Author

coatless commented Feb 3, 2024

@gadenbuie Let's maybe aim to improve the examples.qmd. I can port of some examples inside of the .Rmd. Would you be up for applying the Garrick Xarginan Styling (TM)?

@coatless
Copy link
Collaborator Author

coatless commented Feb 3, 2024

I think one other part here is how do we want to handle deployment of the documentation? Do we also want to add a shinylive variant of the R app as well?

@gadenbuie
Copy link
Owner

gadenbuie commented Feb 4, 2024

Those are both great ideas/questions! Let's tackle those in follow up PRs -- actually it'd probably be worth creating issues for those, and any other next steps from this PR, so we can talk about them as needed.

I'm very okay with this PR going into main soon; everything works enough for people (or just us) to be able to play with it. It'll be easier to have smaller, targeted PRs from here that fill in some of the gaps.

@coatless
Copy link
Collaborator Author

coatless commented Feb 4, 2024

@gadenbuie okay; I've split out the issues into tickets for discussion. List:

Are there any others that should be included?

@gadenbuie gadenbuie marked this pull request as ready for review February 5, 2024 15:24
@gadenbuie
Copy link
Owner

Copy link
Owner

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Thank you again for kicking off this work, I'm excited that countdown will finally be a real, grown-up Quarto extension!

Although we have some more work to do, this is ready for anyone to try out, so I'm going to go ahead and merge.

@gadenbuie gadenbuie changed the title [WIP] Port into Quarto shortcode extension feat: countdown as a Quarto shortcode extension! Feb 5, 2024
@gadenbuie gadenbuie merged commit f06b4ad into gadenbuie:main Feb 5, 2024
5 checks passed
@coatless
Copy link
Collaborator Author

coatless commented Feb 6, 2024

@gadenbuie 🎉 👍

Thanks again for being open for the Quarto & python port 😄

@coatless coatless deleted the quarto-extension branch February 6, 2024 01:20
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.

quarto shortcode
2 participants