Upgrade cicerone to use the latest driver.js@1.3.1

yogat3ch commented 7 months ago

This PR is a relatively substantial overhaul to the cicerone package in order to create a more seamless experience of using the full feature set of driver.js with cicerone. This was accomplished by:

Creating snake case parity between the driver.js documentation and argument names in cicerone such that functionality of particular argument can be clearly determined.
Inclusion of all driver.js configuration options as explicit arguments

Changes

The highlight and initialise methods are now the same function and do what one would expect: create a one-off highlight on the intended element from a reactive context.
The prep_element prepending of # to the element when not a class or ID can be muted with el_as_is = TRUE on Cicerone$step and highlight
Users can supply callback JS code and the callbacks are created internally to the init method in cicerone.js to preserve Shiny and expected driver.js functionality while also allowing users to run their own code.
Users can reference id (the id of the driver), and this.get_driver(id) to call the driver object to run their own API calls inside a callback.

Testing

There is now a demo app in inst/shiny-examples/basic-guide/app.R for use in development and for testing. This app demonstrates a critical subset of the functionality added in this release and can be used for reviewing this PR.

Legacy functionality

R

Legacy functionality was preserved to the greatest extent possible, and deprecation warnings have been provided for all legacy argument names, with substitution and replacement occurring where possible. See deprecate_replace

No longer supported

I was unable to determine what the purpose of the legacy JS code related to tab and tab_id was, so it was omitted. This includes the usage of onHighlightTab. If we wish to retain this, please feel free to open a branch off of this one to re-integrate it.

JohnCoene commented 6 months ago

Sorry it took me so long to come around to reviewing this.

I don't really get

The highlight and initialise methods are now the same function and do what one would expect: create a one-off highlight on the intended element from a reactive context.

As far as I remember these are separate functionalities, the first is to initialise the driver tour, the second to highlight a specific element, regardless of tour.

The tab_id is an argument that allows indicating in which tab (navbarPage) the element to highlight is. This is because within the tour we need to open that tab before highlighting the element, see here

Do you mind looking into the tab_id? The solution to highlight elements in different tabs I came up with is not great, I'm happy if we can improve upon it.

Otherwise looks good, thanks!

EDIT: Also add yourself as contributor to the DESCRIPTION (if you want)

JohnCoene commented 6 months ago

@yogat3ch do you need help with the tab thing? I'm happy to tackle that or help in another way.

yogat3ch commented 6 months ago

Hey @JohnCoene, Yes, I don't understand the rationale for the tab_id arguments because driver.js allows one to just specify a selector to highlight. If you want to reintegrate it, please do so. Right now it's just returning a deprecation warning if used

JohnCoene commented 6 months ago

If a user has a first step in one tab and it's second in another tab we need to first navigate to that tab prior to highlighting the element as it is otherwise not visible on screen.

yogat3ch commented 6 months ago

@JohnCoene Ah, I see. We chained two guides together based on the tab input changing. I didn't realize there was a way to do it within cicerone. How do you account for longer load times between tabs?

JohnCoene commented 6 months ago

We can have multiple cicerone tours in one Shiny app (as you have explained doing yourself) this would break move_iterator for instance.

yogat3ch commented 6 months ago

We can have multiple cicerone tours in one Shiny app (as you have explained doing yourself) this would break move_iterator for instance.

@JohnCoene Are you saying if there were multiple cicerone tours in an app it would break something called move_iterator? I'm not sure what that is, could you elaborate?

It sounds like move_iterator is a varaible of some kind, so are you talking about overlapping variable assignment in the case of multiple tours? Wouldnt that only happen if the tours ran simultaneously? Im not sure anyone would be inclined to do that, if so.

JohnCoene / cicerone