-
Notifications
You must be signed in to change notification settings - Fork 55
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
Allow recording/playing back inputs that do not have a binding #232
Conversation
) | ||
), | ||
value = FALSE | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any real danger in having this default to TRUE
? After running your example I was confused as to why the code generator wasn't working, then I realized I had to check this box...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defaulted to FALSE
because I didn't want people to start recording tests that use this feature, and then be surprised when the testing doesn't work perfectly -- screenshots that don't look right and/or behavior that isn't correct (due to an htmlwidget that needs actual interaction). By requiring users to opt-in, they're more likely to be aware of that.
I do get that the current situation is confusing. I can think of a few different alternatives:
- In the right panel, input events without a binding could be highlighted in red. The drawback is that this isn't helpful for people who encounter it for the first time -- there needs to be some way to discover how to deal with it. Maybe a question mark and tooltip?
- When the user clicks "Save and Quit", it could pop a confirmation dialog if there are any inputs without a binding and the box is unchecked. The dialog could say something roughly like, "There are input events without a binding. If you want them to be saved in the test script, press Cancel, then check the box, then click on Save and Quit again. If you don't want to save them, press OK."
- By default, it could just generate a script with the code, but have it commented out. Then users could fix it by uncommenting the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally got around to testing this out. I'd recommend the second alternative (confirmation dialog) if feasible.
paste0( | ||
"app$setInputs(", | ||
quoteName(event$name), " = ", | ||
processInputValue(event$value, event$inputType), | ||
args, | ||
")" | ||
) | ||
|
||
} else { | ||
if (allowInputNoBinding) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you could get rid of a set of parentheses: else if (allowInputNoBinding) {
I am very excited to see this update! I did some testing and I only have some minor comments:
|
This PR adds the ability to record and play back inputs for which there is not an input binding, as is the case with htmlwidgets that have inputs, like DT and plotly.
When playing back, the input value will be set so that the R Shiny process knows about it, but the corresponding object in the browser will not -- so, for example, in the app below, if a row in the DT is selected during the recording phase, it will not be highlighted during playback. For some applications, this is OK, for others it is not.
Example test app with DT:
The resulting test script looks like this:
Also note that some of the inputs should really be set simultaneously. I believe that
table_rows_selected
,table_row_last_clicked
, andtable_cell_clicked
would all be set on the same "tick" of the JS event loop, so they could be coalesced into a single call to$setInputs()
. It might be possible in the future to automatically detect these cases and coalesce these cases.cc: @rpodcast, @cpsievert