-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
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 |
@gadenbuie yup; I'm aiming for a |
@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). |
@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:
|
@gadenbuie thanks for the invite! I'm not too worried about the re-work. By moving away from 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! |
@gadenbuie we do have a design choice to think of when it comes to the document-level CSS, e.g. How should the styles be set in document-level settings? Option A: Top level option 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" |
Should we allow globally setting options like 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:
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 |
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?
Okay, we'll aim for Option B then to keep consistency across the packages. |
There was a problem hiding this 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!
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()`.
@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 ![]() I'd like to try to get this into the repo this weekend. Given our prior discussions on |
There was a problem hiding this 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!
Co-authored-by: Garrick Aden-Buie <[email protected]>
Co-authored-by: Garrick Aden-Buie <[email protected]>
Co-authored-by: Garrick Aden-Buie <[email protected]>
@gadenbuie Let's maybe aim to improve the |
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? |
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. |
@gadenbuie okay; I've split out the issues into tickets for discussion. List:
Are there any others that should be included? |
Two more follow up tasks: |
There was a problem hiding this 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 🎉 👍 Thanks again for being open for the Quarto & python port 😄 |
Close #27
TODO:
_extension
Mirror whisker write for CSSgetOption(options, key, default)
for check/retrieval/or use default andtryOption()
for just check/retrievalThink about options forprismatic()
c.f. quarto-dev/quarto-cli#8353
Non-ported functions:
prismatic::clr_lighten()
(Show stopper)prismatic::clr_darken()
(depends onclr_lighten()
)prismatic::best_contrast()