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

Fix: Changed inputID to inputId in cookies_banner_server() #77

Merged

Conversation

JT-39
Copy link
Collaborator

@JT-39 JT-39 commented Dec 24, 2024

Brief overview of changes

Changed the typo inputID = ... to inputId = ... in the cookies_banner_server() function.

Why are these changes being made?

The current is a typo and is causing the following error when clicking on "View cookies information" in the cookie banner:

Warning: Error in shiny::updateTabsetPanel: unused argument (inputID = cookies_nav_id)
  81: observe [C:/Users/jtufts/AppData/Local/Temp/Rtmp0YKBTu/renv-package-new-15d87ae47c98/dfeshiny/R/cookies.R#234]
  80: <observer:observeEvent(input$cookies_link)>
   1: shiny::runApp

Detailed description of changes

  • Changed the type inputID = ... to inputId = ... in the cookies_banner_server() function, this part specifically:
shiny::observeEvent(input$cookies_link, {
      # Need to link here to where further info is located.  You can
      # updateTabsetPanel to have a cookie page for instance
      shiny::updateTabsetPanel(
        session = parent_session,
        inputID = cookies_nav_id,
        selected = cookies_link_panel
      )
    })
  • projectId: was added to the .Rproj file automatically by R. Think is new version of R behaviour - but not sure if suitable here?

Issue ticket number/s and link

#75

…ectId auto added to .Rproj - not sure if wanted
Copy link
Contributor

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

Great spot! Thanks for raising this 😄

I've seen the Rproj update recently too, happy enough for it to go in as it's a harmless enough change and does seem to be the new default.

As this is from a fork I'm assuming you can't merged it, so I'll merge this in now as I've tested it out and am happy with it.

I've raised a follow up PR that adds a quick extra check on this link in the tests/test_dashboard so we'll spot if we break it again in the future

@cjrace cjrace merged commit 82b9566 into dfe-analytical-services:main Dec 30, 2024
10 checks passed
@JT-39 JT-39 deleted the fix-cookies-banner-link-id-typo branch January 2, 2025 09:41
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.

2 participants