shinyMobile icon indicating copy to clipboard operation
shinyMobile copied to clipboard

No warning message for deprecated arguments f7Panel(inputId =.... and f7Navbar( left_panel = TRUE ... and f7checkBox is not in version 0.8.0

Open kmezhoud opened this issue 4 years ago • 12 comments

Hi, I think,

  • f7Panel(inputId = ...) is converted to f7Panel(id= ... No Warning message if we use old argument.

  • f7Navbar(left_Panel=TRUE...) is converted to F7Navbar(leftPanel=TRUE...) No Warning message if we use old argument.

  • f7Checkbox is deprecated but f7checkBox does not exist : Only old version is working.

  • new function like f7checkBox used old argument like inputId and not id : I think id is preferred since in f7Panel and other functions, we note inputId converted to id.

shinyApp(
  ui = f7Page(
    title = "Tab layout",
    f7TabLayout(
      tags$head(
        tags$script(
          "$(function(){
               $('#tapHold').on('taphold', function () {
                 app.dialog.alert('Tap hold fired!');
               });
             });
             "
        )
      ),
      panels = tagList(
        f7Panel(inputId = "panelLeft_id", title = "Left Panel", side = "left", theme = "light", "Blabla", effect = "cover"),
        f7Panel(inputId = "panelRight_id", title = "Right Panel", side = "right", theme = "dark", "Blabla", effect = "cover")
      ),
      navbar = f7Navbar(
        title = "Tabs",
        hairline = FALSE,
        shadow = TRUE,
        left_panel = TRUE,
        right_panel = TRUE
      ),
      f7Tabs(
        animated = FALSE,
        swipeable = TRUE,
        f7Tab(
          tabName = "Tab 1",
          icon = f7Icon("email"),
          active = TRUE,
          f7Shadow(
            intensity = 10,
            hover = TRUE,
            f7Card(
              title = "Card header",
              f7Stepper(
                "obs1",
                "Number of observations",
                min = 0,
                max = 1000,
                value = 500,
                step = 100
              ),
              
              f7Checkbox(InputId = "ck1", label = "chechouk", value = TRUE),
              plotOutput("distPlot1"),
              footer = tagList(
                f7Button(inputId = "tapHold", label = "My button"),
                f7Badge("Badge", color = "green")
              )
            )
          )
        )
      )
    )
  ),
  server = function(input, output) {
    output$distPlot1 <- renderPlot({
      dist <- rnorm(input$obs1)
      hist(dist)
    })
    
    output$distPlot2 <- renderPlot({
      dist <- switch(
        input$obs2,
        norm = rnorm,
        unif = runif,
        lnorm = rlnorm,
        exp = rexp,
        rnorm
      )
      
      hist(dist(500))
    })
    
    output$data <- renderTable({
      mtcars[, c("mpg", input$variable), drop = FALSE]
    }, rownames = TRUE)
  }
)



Error in f7Checkbox(InputId = "ck1", label = "chechouk", value = TRUE) : 
  impossible de trouver la fonction "f7Checkbox"

kmezhoud avatar Mar 11 '21 13:03 kmezhoud

Please give the current github version a try. Some of the issues probably are corrected.

  • f7Panel(inputId = ...) is converted to f7Panel(id= ...

Not clear about this one

  • f7Navbar(left_Panel=TRUE...) is converted to F7Navbar(leftPanel=TRUE...) Looks like this has been corrected, I cannot find it

  • f7Checkbox is deprecated but f7checkBox does not exist

There are sever more places where depreciation warning is issued. I will create a pull request to take some of the workload from David.

  • impossible de trouver la fonction "f7Checkbox"

It was not exported (will be in pull request)

  • new function like f7checkBox used old argument like inputId and not id

I believe inputId is the preferred one, since it is in line with standard shiny. Up to david to decide. updateTabsetPanel in shiny has inputId.

dmenne avatar Mar 11 '21 14:03 dmenne

@dmenne Thanks for the support. Have a look at the rc-1.0.0 branch where some of the above problems may be fixed.

DivadNojnarg avatar Mar 11 '21 14:03 DivadNojnarg

Indeed, look at this: https://github.com/RinteRface/shinyMobile/commit/a2d039aef34206a7183c6a67937023aff64163e8 the missing export checkbox is part of the rc-1.0.0

DivadNojnarg avatar Mar 11 '21 14:03 DivadNojnarg

I saw rc-1.0.0, but I did not bother to look at it, because changes where older than in the master branch. I am a bit confused now what is the right base version (e.g. with your yesterday's changes), so I will temporarily not prepare a pull request.

I think there is a general problem with the design of your deprecated functions. The new version should have the correct code, and the deprecated should call the new version (possibly with corrected parameters). Then later you simply could add defunct.

I did not want to change this in the last pull request globally, I did it only for one case.

In test files, context() is no longer recommended when you stick to the naming conventions from utils (I believe you did).

https://www.tidyverse.org/blog/2019/04/testthat-2-1-0/#context-is-now-optional

dmenne avatar Mar 11 '21 15:03 dmenne

I did a check in the dplyr source code. Hadley puts all deprecated functions into a separate documentation. I think that this is a clever move, because when reading a documentation page with subtle changes as you have, I am often confused on a quick look which one to use. You package is new enough to allow for such changes. lazy_deprec calls a function in the lifecycle package which does not have a large dependency burden.

#' @rdname se-deprecated #' @inheritParams tally #' @export tally_ <- function(x, wt, sort = FALSE) { lazy_deprec("tally")

wt <- compat_lazy(wt, caller_env()) tally(x, wt = !!wt, sort = sort) }

dmenne avatar Mar 11 '21 15:03 dmenne

As I don't have time to pursue it, I'll probably merge rc-1.0.0 in the master to restart fresh so that you can prepare your PR (just need to check whether it's feasible now).

DivadNojnarg avatar Mar 11 '21 15:03 DivadNojnarg

Hi, id seems to be the future syntax instead inputId, please see the trend of changes here.

In version rc-1.0.0, there is always the same issues:

1- We can use f7Panel(inputId....) without warning message of deprecated inputId. 2- The new f7Checkbox is working but we receive a warning message saying: Warning: f7checkBox will be removed in future release. Please use f7Checkbox instead. 3- It is always possible to use f7Navbar(left_Panel=TRUE...) without warning message but the panel do not appear.

Thanks,

kmezhoud avatar Mar 11 '21 19:03 kmezhoud

id or inputId: I see your point, @kmezhoud , there is some confusion about the future here. I would strongly prefer inputId because it is used by Shiny.

Your other comments are probably resolved. As David mentioned, rc-1.0.0 is not current (last changes 2 months ago). So we should wait for the promised merge.

Let's hope that Novartis provides some funding to maintain the code. This is too important a package to leave it to private night shifts to be maintained.

dmenne avatar Mar 12 '21 08:03 dmenne

Regarding the convention, the direction I am taking ({shinydashboardPlus}, {bs4Dash}):

  • inputId will be used for all primary inputs, that is a sliderInput() for instance.
  • id will be used for all non primary inputs like f7Panel(). Understand, a panel is not a real input by itself like would be a numericInput(). We just use the id to toggle its state and do other things.

I will not change the original{shiny} functions like tabsetPanel where they use inputId instead of id, even though I don't agree with the notation.

DivadNojnarg avatar Mar 12 '21 11:03 DivadNojnarg

Thanks for the id/inputId clarification.

I am working on a pull request to correct the deprecation calls, as done already for insertF7Tab and following the accepted answer here.

Please advise:

  • Should the documentation of deprecated functions be moved to a separate rdname or kept as is?
  • Should I wait for your merging rc before proceeding?

dmenne avatar Mar 12 '21 13:03 dmenne

In a separate rdname would be cleaner. I'll handle the conflicts no worry.

DivadNojnarg avatar Mar 12 '21 17:03 DivadNojnarg

@dmenne I just merge rc-1.0.0 into master. I'll continue the upgrade later.

DivadNojnarg avatar Mar 14 '21 00:03 DivadNojnarg