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

Add user tours in Advanced Settings #17

Merged
merged 13 commits into from
May 9, 2021

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented May 7, 2021

References

Task

  • add schema based on Declarative Tours? #2
  • add settings machinery
  • handle updating/removing tours/menuItems
  • add to README
    • features
    • how to build a tour in Advanced Settings
    • how to disable user tours (and the default tours)
  • validate on binder Binder
  • tests
  • upgrade react-joyride to 2.3.0, remove third-party typings
  • update use of Placement in steps (where it can also be auto and center)
  • script to manage updating the joyride schema when it changes (is currently old)
    • would prefer to do in python 🤷

Follow-on

I started going down the rabbit hole of executing commands based on the signals... would be super fun, but we can easily push that to another PR.

Screens

Screenshot from 2021-05-07 02-22-09

@bollwyvl bollwyvl changed the title Add user tours Add user tours in Advanced Settings May 7, 2021
@fcollonval
Copy link
Member

Awesome feature 🚀 ❤️

One quick question, would it make sense to add a second plugin that activates this feature (aka loading the settings and adding the tours to the manager)? The idea behind is for admin to be able to deactivate this feature.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 7, 2021

Yeah, that's a good angle.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 7, 2021

Split into:

  • jupyterlab-tour:plugin, just the manager no tours
  • jupyterlab-tour:default-tours, welcome, notebook
  • jupyterlab-tour:user-tours, the settings junk

Took some doing, and did theoretically change the top level API, as it changed a number of concrete types to reference interfaces.

@fcollonval
Copy link
Member

Thanks a lot @bollwyvl

This is really a nice structure improvement.

For the type changes, it looks that the main change is the use of the interface ITourHandler instead of the concrete type. So this should not bring trouble on downstream extensions, does it?

Let me know when you are ready with this one.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 7, 2021

Cool, thanks for the review. Looks like i gotta fix some tests, which is fine, though I'll have to mock settingsmanager, probably, as it's not exactly a lightweight chain.

I don't think the Handler -> IHandler is going to break anybody's stuff. I can actually probably revert those signature changes, if the manager/handler stuff stays in core, but it semi-dirtily references the WELCOME_ID. However, I think they are good from an API contract point of view. Also, I strongly feel that the user-focused thing should be enabled by default, but that could be surprising to a current deployer. In light of both of these, I think a 4.0 is probably going to be warranted, but certainly a 3.1.

I ran into some issues with generating the schema, and can't find my hacked script... will try a little bit more this weekend.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 8, 2021

I think i am going to extract most of the changes to ITourManager out to IUserTourManager... though firing off the tests with what's in might be good to make sure i didn't mess up the tests too much...

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 8, 2021

Yep, pretty happy with the new shape: it removes some weird timing things, etc.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 9, 2021

@fcollonval no more changes planned from this side, but happy to address any comments!

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @bollwyvl

I have two minor comments. Then we are good to go.

src/userTourManager.ts Outdated Show resolved Hide resolved
"jupyterlab-tour:user-tours": true,
"jupyterlab-tour:default-tours": true,
}
}
```

Copy link
Member

Choose a reason for hiding this comment

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

What about adding the alternative with the command line. Something like


Alternatively, you can also use the following commands:

jupyter labextension disable "jupyterlab-tour:user-tours"
jupyter labextension disable "jupyterlab-tour:default-tours"

To enable the extension, use jupyter labextension enable ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, 66b0f95

bollwyvl and others added 2 commits May 9, 2021 12:21
@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 9, 2021

@fcollonval thanks, added both ideas.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks again for this great feature and the quick reply to the review.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 9, 2021

boo test fail, looking...

@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 9, 2021

fix from cefa2d7 passes locally 🎉

@fcollonval fcollonval merged commit f1913ae into jupyterlab-contrib:master May 9, 2021
@bollwyvl
Copy link
Contributor Author

bollwyvl commented May 10, 2021 via email

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.

Build tours in Advanced Settings/overrides.json?
2 participants