-
Notifications
You must be signed in to change notification settings - Fork 26
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: content_render()
can take a variant_key
argument to render non-default variants.
#289
Conversation
9980ca3
to
9db4f9e
Compare
I'm not sure why, but this PR has been sitting unreviewed for almost a month. The I don't like the implementation as written. Currently it requires a content item and variant key to render a variant, but in In the future, we're also going to move away from the The real trouble I'm having making this fit into the current type system of the package is just around the return value from this function. I was going to push an update to return a Even more confusing because |
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.
Oops, apparently this has been sitting there in "pending" feedback this whole time 🤦
One suggestion but otherwise I think this is fine. We can fix the R6 mess later, seems out of scope for this change.
@@ -978,16 +979,20 @@ get_content_permissions <- function(content, add_owner = TRUE) { | |||
#' } | |||
#' | |||
#' @export | |||
content_render <- function(content) { | |||
content_render <- function(content, variant_key = NULL) { |
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.
If you do this, you don't need the if
below.
content_render <- function(content, variant_key = NULL) { | |
content_render <- function(content, variant_key = "default") { |
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.
Yeah, I realize, but I included the if
statement because I would rather have NULL
represent the "no variant key provided" case than "default"
— that feels more… idiomatically expected to me.
This PR adds a new argument to
content_render()
,variant_key
, which isNULL
by default. If a value is passed in, this is used to look up a non-default variant to render. By default, if no value is provided forvariant_key
, the default variant is rendered.content_render
now returns aVariantTask
object, not aContentTask
.ContentTask
inherited fromContent
, andVariantTask
inherits fromVariant
, which inherits fromContent
. So, it's basically aContent
object withVariant
data and aTask
subfield.get_variant()
. Before, you had to pass in the magic string"default"
tokey
. I tried changing the default tokey = NULL
, and having that return the default variant. I wound up rolling that change back to how it was before, because it strikes me as a little weird thatget_variant(content)
, with no arguments, returns the default variant.