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

Adding button with "reload" functionality. #1

Merged
merged 6 commits into from
Aug 16, 2020

Conversation

danoobae
Copy link
Contributor

  • Added button with "reload" functionality.
  • Added testthat with the new button.

Copy link
Member

@tylerlittlefield tylerlittlefield left a comment

Choose a reason for hiding this comment

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

Overall looks really good! A few changes need to be made to the test so that the builds pass. Additionally, we can take this opportunity to make a utils.R file for utility functions used more than once.

#' Reload button with button and icon animations
#'
#' Animate a reloadButton and it's icon using
#' [Hover.css](https://github.com/IanLunn/Hover)
Copy link
Member

Choose a reason for hiding this comment

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

Since reloadButton isn't an actual button type from shiny, let's reword:

Animate a reload button and it's icon using Hover.css. Note that a reload button is just a shiny::actionButton with onClick behavior to reload or refresh a web browser.

)
}

validateIcon <- function (icon) {
Copy link
Member

Choose a reason for hiding this comment

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

validateIcon is now copied and pasted twice. Let's create a file utils.R, you can use usethis::use_r("utils"), then add validateIcon to the file.

test_me <- hover_reload_button("id", "label")

# Test 1: attribs type is a button
expect_match(
Copy link
Member

Choose a reason for hiding this comment

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

I think expect_equal would be a better test to use in this example.

)

# Test 2: names on attribs list contains onClick
expect_match(
Copy link
Member

Choose a reason for hiding this comment

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

I think expect_true(any("onClick" %in% names(test_me$attribs))) would work well here.

)

# Test 3: Look for javascript command
expect_match(
Copy link
Member

Choose a reason for hiding this comment

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

I think expect_equal would be a better test in this example, expect_equal(test_me$attribs$onClick, "location.reload();").

@@ -0,0 +1,33 @@
context("Reload functionality")

test_that("Test for three components in the button. Type is a 'button'.",
Copy link
Member

Choose a reason for hiding this comment

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

Let's reword to:

"hover_reload_button has expected structure"

FYI, I think the builds are failing because test_that is expecting a single vector for the description, e.g. the desc param.

@@ -0,0 +1,33 @@
context("Reload functionality")
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the context call, the tidyverse team has deprecated this function and actually discourages using it.

@tylerlittlefield tylerlittlefield merged commit 7ac2bf1 into r4fun:master Aug 16, 2020
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.

None yet

2 participants