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

Anomalous behaviour for disabledDates option in updateAirDateInput function #379

Closed
javierdiegof opened this issue Mar 24, 2021 · 3 comments

Comments

@javierdiegof
Copy link

javierdiegof commented Mar 24, 2021

Hello, hope you're doing fine. Thanks a lot for all your work in this wonderful package.

General explanation

This issue deals with problems I've encountered recently when trying to update the disabled dates in an airDatepickerInput using the updateAirDateInput function. The errors occur on some edge cases, particularly when trying to update the disabled dates in the selector to have either zero disabled dates (that is, no disabled dates), or just one.

Demo

I've created a little demo that shows the tricky cases, it just creates a datepicker object with some disabled dates and updates them:

library(shiny)
library(shinyWidgets)
library(magrittr)

# Generate all dates of march 2021
march_days <- seq(as.Date("2021-03-01"), as.Date("2021-03-31"), by = "days")
march_days <- sapply(march_days, function(x){toString(x)})
# The disabled ones will be all of those whose day is not multiple of 5
disabled_days <- setdiff(march_days, c("2021-03-05", "2021-03-10", "2021-03-15",
                                       "2021-03-20", "2021-03-25", "2021-03-30"))

ui <- fluidPage(
    titlePanel("Updating an airDatepicker"),
    sidebarLayout(
        sidebarPanel(
            # Setting the initially disabled
            airDatepickerInput("date_picker", "Date", 
                               minDate = "2021-03-01", 
                               maxDate = "2021-04-1", 
                               disabledDates = disabled_days,
                               update_on = "close", 
                               range = TRUE),
            actionButton("button", label = "Update picker")   
        ),
        mainPanel(
           textOutput("text")
        )
    )
)

server <- function(input, output, session) {
    # Show the dates
    output$text <- renderText({
        input$date_picker
        input$date_picker_button
        toString(isolate(input$date_picker))
    })
    
    # Update the disabled dates
    observeEvent(input$button, {
        print("clicking button")
        # The tricky cases
        updateAirDateInput(session, "date_picker", options = list(
            # disabledDates = c()
            # disabledDates = NULL
            # disabledDates = character(0)
            # disabledDates = "2021-03-24"
        ))
    })
}

shinyApp(ui = ui, server = server)

You can uncomment each of the possibilities and run the app to analyze the behaviour.
In order to explain the errors more easily, I will show the code that deals with the update of the disabledDates on the JS side, it is inside the inst/assets/air-datepicker2/datepicker-binding.js file:

receiveMessage: function(el, data) {
    ...
      var disabledDates = [];
      if (data.options.hasOwnProperty("disabledDates")) {
        disabledDates = data.options.disabledDates;
      }
      var highlightedDates = [];
      if (data.options.hasOwnProperty("highlightedDates")) {
        highlightedDates = data.options.highlightedDates;
      }
      data.options.onRenderCell = function(d, type) {
        if (type == "day") {
          var disabled = false,
            highlighted = 0,
            formatted = getFormattedDate(d);

          disabled = disabledDates.filter(function(date) {
            return date == formatted;
          }).length;

          highlighted = highlightedDates.filter(function(date) {
            return date == formatted;
          }).length;

          var html = d.getDate();
          var classes = "";
          if (highlighted > 0) {
            html = d.getDate() + '<span class="dp-note"></span>';disabledDates
            classes = "airdatepicker-highlighted";
          }

          return {
            html: html,
            classes: classes,
            disabled: disabled
          };
        }
      };
     ...
  }

Current behaviour

At the moment, for the different cases the app behaves as follows:

  • c() and NULL: both are NULL values and therefore behave equally. The browser will receive the value and set the highlightedDates variable to Javascript's null, which will cause an error when filtering the dates. I don't know what happens when nothing is returned from the onRenderCell function, but the end result is that all elements are enabled.
  • character(0): The browser receives this value as an empty javascript vector and therefore all values are enabled.
  • "2021-03-24": The browser will receive the value and set the highlightedDates variable to string variable. Just as with null this crashes the code and leaves the app with all values enabled.

Expected behaviour

I think there is room for discussion on how the NULL values should be interpreted, but in any case the code should not crash and therefore I consider the current current code to be incomplete (it has a bug). A possibility for how the values could behave is the following one:

  • c() and NULL: I think are are two valid interpretations of NULL values for the update method. Stock Shiny update functions interpret NULL values by ignoring them, which leaves the widget as it is right now, without updating them. Making the code work that way would be somewhat complicated as the Javascript code stands right now. Another possible way to interpret NULL values is just to treat them just like the c(0), that could be done in either the R side (by converting NULL option to c(0) before sending it to the browser) or on the JS side (look at the code below).
  • character(0): The way it is handled currently is correct.
  • "2021-03-24": It should set the date specified as the only disabled date.

Possible solution

Although somewhat of a hotfix, a solution to both of the crashes would be to set the specific safeguards on the JS side for the possibilities that crash the code:

var disabledDates = [];
if (data.options.hasOwnProperty("disabledDates")) {
  var optDates = data.options.disabledDates;
  // R's NULL will be interpreted as enabling all dates 
  if (optDates === null) {
    disabledDates = [];
  }
  // Correct handling of the single string
  else if (typeof(optDates) === "string") {
    disabledDates = [optDates];
  }
  else{
    disabledDates = opDates;
  }
}

As I mentioned before, the interpretation of the values and how to deal with them could be done in several different ways. A solution to the errors could be implemented on the R side too and the semantics of NULL could be different.

Thanks in advance :).

@pvictor
Copy link
Member

pvictor commented Mar 26, 2021

Thank you for the thorough report !

I've updated the function to work with following behaviors:

  • NULL or c(): leave the input unchanged
  • charater(0) remove all disabled dates
  • "2021-03-24" transform to list in R so that it's considered as an array in JS

Let me know how it works for you.

Victor

@javierdiegof
Copy link
Author

Thanks a lot for the quick response.
I'll have a look at the code and will check that everything is correct.
I'll close the issue if I find no errors, thanks again!

@javierdiegof
Copy link
Author

javierdiegof commented Mar 26, 2021

The code works great! I liked the dual approach (on both JS and R) you used to solved the problem.
Thank you very much 😊

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

No branches or pull requests

2 participants