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: content_render() can take a variant_key argument to render non-default variants. #289

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

toph-allen
Copy link
Collaborator

@toph-allen toph-allen commented Aug 2, 2024

This PR adds a new argument to content_render(), variant_key, which is NULL 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 for variant_key, the default variant is rendered.

  • content_render now returns a VariantTask object, not a ContentTask. ContentTask inherited from Content, and VariantTask inherits from Variant, which inherits from Content. So, it's basically a Content object with Variant data and a Task subfield.
  • I toyed around with changing the way you specify the default variant in get_variant(). Before, you had to pass in the magic string "default" to key. I tried changing the default to key = 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 that get_variant(content), with no arguments, returns the default variant.

@toph-allen toph-allen force-pushed the toph/content-render-variant-id branch from 9980ca3 to 9db4f9e Compare August 5, 2024 15:00
@toph-allen toph-allen marked this pull request as ready for review August 5, 2024 16:09
@toph-allen
Copy link
Collaborator Author

I'm not sure why, but this PR has been sitting unreviewed for almost a month. The connectapi release will happen imminently, and I'd either like to merge this or put it on hold.

I don't like the implementation as written. Currently it requires a content item and variant key to render a variant, but in connectapi's currently schema, it should really also accept a variant. However, variants also have the render() method to render them; this seems duplicative.

In the future, we're also going to move away from the Variant class being an equivalent to Content.

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 VariantTask when the object rendered is a variant, and a ContentTask when it's the default variant, but then, what if they pass in a content item and a variant key? Return a VariantTask, obviously. What if they specify the default variant with a variant_key or pass in a Variant class object representing the default variant?

Even more confusing because VariantTask inherits from Variant and ContentTask inherits from Content — it's like the current type system is trying to have it both ways and return both a task object and the object it was passed, kinda hacking in multiple inheritance even though R6 doesn't support it.

Copy link
Collaborator

@nealrichardson nealrichardson left a 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) {
Copy link
Collaborator

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.

Suggested change
content_render <- function(content, variant_key = NULL) {
content_render <- function(content, variant_key = "default") {

Copy link
Collaborator Author

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.

@toph-allen toph-allen merged commit bfefb00 into main Sep 3, 2024
19 checks passed
@toph-allen toph-allen deleted the toph/content-render-variant-id branch September 3, 2024 16:30
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.

3 participants