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 guided tour #164

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

added guided tour #164

wants to merge 2 commits into from

Conversation

biplab-sutradhar
Copy link
Contributor

@biplab-sutradhar biplab-sutradhar commented Nov 6, 2024

added guided tour feature

Closes #150

@biplab-sutradhar
Copy link
Contributor Author

biplab-sutradhar commented Nov 6, 2024

Closes #150

TDH: this comment does not work because the "Closes" comments must be in the first comment of the PR

element: "#" + id,
popover: {
title: geom.charAt(0).toUpperCase() + geom.slice(1),
description: `${helpText} Data are shown for the current selection of: ${showSelected}. Click to change selection of: ${clickSelects}.`
Copy link
Collaborator

@tdhock tdhock Nov 7, 2024

Choose a reason for hiding this comment

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

this looks like a good first step thanks!
when showSelected is an array ['var1','var2'], does this work?
also it could be missing or empty array [] (same for clickSelects, and help)
so in that case we would want to omit each missing part altogether.

@@ -185,6 +185,24 @@ var animint = function (to_select, json_file) {

var add_geom = function (g_name, g_info) {
// Determine if data will be an object or an array.
// added geom properties in steps array
var geom = g_info.geom;
var id = "plot_" + geom + "Plot";
Copy link
Collaborator

@tdhock tdhock Nov 7, 2024

Choose a reason for hiding this comment

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

I'm not sure this selection based on id is correct. can you please use a selection based on g_info.classed?
see classes of <g> elements below
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if i use g_info.classed then it shows unexpected behaviors
image
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is good! what is unexpected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For Geom2_vline_vlinePlot please

  • please change "Geom2_vline_vlinePlot" to the value of g_info.classed
  • remove "Data are shown for the current selection of ." no text like this should be shown if there are no showSelected variables.
  • remove "Click to change selection of: ." no text like this should be shown if there is no clickSelects variable.

@@ -12,6 +12,8 @@
<script type="text/javascript" src="vendor/jquery-1.11.3.min.js"></script>
<script type="text/javascript" src="vendor/selectize.min.js"></script>
<link rel="stylesheet" type="text/css" href="vendor/selectize.css" />
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/driver.js.iife.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please copy these to inst/htmljs/vendor? and then use relative links here? (as with jquery etc above)
this approach is more stable (all files hosted on same server, don't need to rely on external cdn)


element.append("button")
.text("Start Tour")

Copy link
Collaborator

Choose a reason for hiding this comment

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

please undo addition of empty line in the middle of statements like this

.text("Start Tour")

.on("click", function() {
startTour();
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is the only place that startTour is called, I think it not worth adding the startTour function. can you please move the contents of startTour inside this anonymous function?

@tdhock
Copy link
Collaborator

tdhock commented Nov 7, 2024

looks like a good start, thanks very much!
please add a test case in a new file, test-renderer-driverjs.R
make sure to simulate a click on the Start Tour button with clickID(), then call getHTML() and use expect_identical to ensure that the expected help text is displayed, simulate a click on the Next button, another expect_identical, etc

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.

guided tour of each animint
2 participants