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

Added id's to menuItems and menuSubItems #128

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

metanoid
Copy link

@metanoid metanoid commented Feb 27, 2016

Please see issue #114

I could not understand how to implement the suggestion of user @Funnng as shown
here

To this end, I tried my hand at adding id's directly to the menuItem and subMenuItem functions in the shinydashboard code.

I ran the tests, and everything seems to be working. However, I have absolutely no idea what I am doing. Do not merge this pull request until thorough testing has been done by competent persons.

Thanks.

@dmpe
Copy link
Contributor

dmpe commented Feb 29, 2016

You need to fix R docs (in man folder) in order to pass travis tests. See here: https://travis-ci.org/rstudio/shinydashboard/builds/112231436#L1353

If you are using rstudio (i hope) + devtools package, then just do CTRL+SHIFT + D. And then push again.

Hopefully Winston (@wch) will take a look on both PR soon. (I would like to open a third one but waiting to have the first one being merged).

@metanoid
Copy link
Author

metanoid commented Jul 6, 2016

Also added id to box, valueBox, and infoBox

@danwwilson
Copy link

This would be great to have merged into the dashboard. I am trying to add Google Analytics tracking to a shiny dashboard and it is proving incredibly difficult to do so without IDs being available. It would be great to add this to the tabPanel object too so I can track if someone clicks on the various panels.

@pauldx
Copy link

pauldx commented Jan 19, 2017

Did we fixed the tabpanel id identification ? Current issue is that tabpanel has random value with no id and can't be associated with event action and hence can't logged that event click information in google analytics.

@wch wch added the targeted label Jan 26, 2017
@wch wch mentioned this pull request Jan 26, 2017
2 tasks
@bborgesr bborgesr added ready and removed targeted labels Feb 24, 2017
@bborgesr bborgesr added backlog and removed ready labels Apr 27, 2017
@Eli-Berkow
Copy link

Eli-Berkow commented Mar 13, 2018

@metanoid, I came across an issue with the infoBox id in your version of shinydashboard today.

Running this example:

library(shiny)
library(shinydashboard)

ui <- dashboardBody(
  infoBox(title = "Test")
)

server <- function(input, output) {}

shinyApp(ui = ui, server = server)

throws the following error:

Error in tag("div", list(...)) : promise already under evaluation: recursive default argument reference or earlier problems?

The app works if we explicitly add an id as in id = "test".

The documentation has:

id An optional id for the element

so it seems to be behaving strangely.

Copy link

@Eli-Berkow Eli-Berkow left a comment

Choose a reason for hiding this comment

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

Hi Colin
It seems you have id = id for the infoBox where it should be id = NULL as in the other cases which explains the recursive error

Good catch - thanks!
@metanoid
Copy link
Author

Thanks @Eli-Berkow , fixed - good catch!

@RoyMudie
Copy link

RoyMudie commented Aug 8, 2019

Any chance of this getting merged. Looks like a great addition and becomes very nessecary as you try to add any kind of functionality beyond the basic stuff

@CLAassistant
Copy link

CLAassistant commented May 4, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ metanoid
❌ soar-tm-discovery
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants