Thumbnail #319
Thumbnail #319
Conversation
|
Fixes #250 Bonus: dev version of
Not sure if there's a better way to get the thumbnails on the unpublished viz's, but it's a step in the right direction |
|
Changes Unknown when pulling 7da50bd on ldecicco-USGS:thumbnail into ** on USGS-VIZLAB:master**. |
|
Changes Unknown when pulling a39098b on ldecicco-USGS:thumbnail into ** on USGS-VIZLAB:master**. |
R/publish.R
Outdated
| dims <- checkThumbCompliance(width, height, file_size$size, thumbType) | ||
| } | ||
|
|
||
| viz[['url']] <- pastePaths(getVizURL(), viz[['relpath']])#need to add slash between? |
There was a problem hiding this comment.
the answer to the comment is no.
R/publish.R
Outdated
| #' @param size numeric file size in bytes | ||
| #' @param thumbType char Type of thumbnail, could be "facebook", "twitter", "landing", "main" | ||
| checkThumbCompliance <- function(width, height, size, thumbType){ | ||
| match.arg(thumbType, c("facebook","twitter","main","landing")) |
There was a problem hiding this comment.
👍 but maybe
thumbType <- match.arg(thumbType, c("facebook","twitter","main","landing"))
because match.arg won't complain if thumbType is 'face' (and that's intended to be a feature, not a bug), but thumbType won't be translated to facebook without the assignment.
Alternatively, if you want to take a harder line:
stopifnot(all(thumbType %in% c("facebook","twitter","main","landing"))
R/publish.R
Outdated
| #' Get thumbnail info for sematics | ||
| #' @param context list, should be just info section | ||
| #' @param thumbnails list, taken from full context | ||
| update_thumbnails <- function(context, thumbnails){ |
There was a problem hiding this comment.
seems kinda silly to worry about it b/c we're doing snake_case for actual vizzies, but vizlab follows camelCase...
R/publish.R
Outdated
|
|
||
| repos <- getRepoNames(viz[['org']]) | ||
| viz_info <- lapply(repos, getVizInfo, org=viz[['org']]) | ||
| viz_info <- lapply(repos, getVizInfo, org=viz[['org']], dev) |
There was a problem hiding this comment.
last arg could be viz[['dev']] instead of dev if following suggestion below at https://github.com/USGS-VIZLAB/vizlab/pull/319/files#diff-58996d4f2f3624e968608c612deab004R15
R/publishLandingPage.R
Outdated
|
|
||
| file.copy(from=system.file('landing', package="vizlab"), to=getwd(), | ||
| recursive = TRUE, overwrite = TRUE) | ||
| oldwd <- getwd() | ||
| setwd('landing') | ||
| on.exit(setwd(oldwd)) | ||
|
|
||
| publish('landing') | ||
| publish('landing', dev=dev) |
There was a problem hiding this comment.
Having a dev version of publish is an awesome idea. I'd prefer a slightly different implementation (though am open to discussing) - specifically, Jordan Walker worked so hard to make fetch/process/visualize/publish be single-argument functions that I'd like to us to try to preserve that.
One way to do this would be to replace this line with:
viz <- as.viz('landing')
viz <- as.publisher(viz)
viz$dev <- dev
publish(viz)and then to have
viz_info <- lapply(repos, getVizInfo, org=viz[['org']], viz[['dev']])here: https://github.com/USGS-VIZLAB/vizlab/pull/319/files#diff-b2d70d609ac0e9c4f742e4a599d45f19R422
There was a problem hiding this comment.
Oh! I didn't realize leaving out the ...'s was by design. I actually assumed it was an oversight to not have them there. OK, I'll make the change
| } else { | ||
| return() | ||
| publish_date <- as.Date(viz.yaml$info$`publish-date`) | ||
| is_published <- TRUE |
R/publish.R
Outdated
|
|
||
| repos <- getRepoNames(viz[['org']]) | ||
| viz_info <- lapply(repos, getVizInfo, org=viz[['org']]) | ||
| viz_info <- lapply(repos, getVizInfo, org=viz[['org']], dev) | ||
| names(viz_info) <- repos | ||
| # rm null | ||
| viz_info <- viz_info[!sapply(viz_info, is.null)] |
There was a problem hiding this comment.
I don't think this is the PR or sprint where we should do this, but I'm wondering: Is it possible that publishLandingPage could eventually call vizmake() after creating and changing into the landing directory? That would allow us to use remake to publish the thumbnails and other resources rather than having publish calls sprinkled throughout the publish functions...what are your thoughts while this is all fresh in your mind, @ldecicco-USGS ?
| for: landing | ||
| thumbType: landing | ||
| title: "Landing thumbnail" | ||
| alttext: "Alt text thumbnail" |
There was a problem hiding this comment.
if/when we write some formal tests for this functionality, what are the important features to capture? thumbnail pngs should get copied into target? thumbnails should appear in target header?
There was a problem hiding this comment.
There are some tests in the publish section:
test_that("Thumb publisher works", {
expect_error(publish("facebook-thumb")) #incorrect dimensions
publish('landing-thumb')
expect_true(file.exists('target/images/landing-thumb.png'))
})
There was a problem hiding this comment.
cool, and oops, i forgot that tests might already exist and was only looking at files changed in this PR. silly me - you'd think i'd remember given that my checks of some branches have been breaking on this test for a day or two =).
| #' @param thumbType char Type of thumbnail, could be "facebook", "twitter", "landing", "main" | ||
| checkThumbCompliance <- function(width, height, size, thumbType){ | ||
|
|
||
| stopifnot(all(thumbType %in% c("facebook","twitter","main","landing"))) |
There was a problem hiding this comment.
you've got me questioning if I should replace all my match.arg calls I have spewed about my package coding
There was a problem hiding this comment.
Hmm. I do think partial matching (e.g., 'face' to 'facebook') is sort of nice, and is consistent with behavior in other base R functions. And you do get an error on an ambiguous match with match.arg, so it's not super dangerous that way. But it's true that someone could have passed in a thumbType of 'face' and then seen the surprising behavior that the if('facebook') code block never got executed. In general I quite like the x <- match.arg(x, ...) pattern - that one might be appropriate in other packages where you've used match.arg, while in vizlab our reliance on syntax makes this a good place for a hard stopifnot.
|
Changes Unknown when pulling 0fafc65 on ldecicco-USGS:thumbnail into ** on USGS-VIZLAB:master**. |
No description provided.