Implement Tour UI module

As config entities is simple to add implementation for ListController as all Configurables does in core

Here's a first pass. Updating the issue summary with remaining tasks.

Added remaining tasks.

Includes somes tests from me, and other changes from @nick_schuch.

Added a delete form and tests for it.

Fixed the "delete" button on the tour edit form, as well as the destination of the "cancel" link, and adjusted the tests to prevent random failures.

Working on a proposal for some basic tips manipulation, then I'm done.

Okay, here's the list controller. I've used Views as a pattern.

Why does this need a management interface? A tour building UI is totally contrib land, we should just provide the right API's so it can be easily extended. I see little point to actually providing a UI for this in core, we have little idea what the requirements are for site builders/module maintainers to use this effectively, for example this patch really only allows placing tour - it does not allow creating the contents, to me that renders this pretty much useless.

This can easily live in configuration files, for just core and contrib can take a much better iterative approach at building a great UI for this (starting in clean up phase, to figure out how a feature should be used is not a good idea - and will lead to interfaces that are useless, aka Dashboard). Core should not be responsible for everything, and this is a place where contrib can do great things providing a way to make steps, contents, in different languages, roles etc.

This will likely not end up in core and will be in contrib, but already it has exposed some bugs in tour module. And it will help determine if the APIs are robust enough.

If once this is done, it is decided that this will live in contrib, we can leave the code fixes and write non-UI tests.

Also, you can't add or edit tips yet because we're not done. And that IS an API addition only possible from within core.

@tim.plunkett That sounds like a fine idea, I just wanted to make clear I dont see this as a contrib use case.

adding docs infra tag

reroll to make it work after the info.yml change.

Currently tips get removed if they are not bound to an element id. If you add a UI and the id or data attribute is not required you need to be able to show what those guys added.

(edit) patch for tour.js is in a separate issue #1938032: Simplify tour.js so it'll get out of this patch at some point.

I've just tried the patch above at #15 and it did allow me to create a useful Tour. It's a REALLY great feature!

I didn't see any of the items in the admin menu (like admin/config/user-interface/tour) so had to enter them directly as links. Once I di this, I was able to add a Tour etc.

I'll have a bit of a look as to why they are missing...

Same as #15, just left hook_menu so that links will show up in the admin page.

This incorporates some of the changes in #1932048: Clean up tour ConfigEntity architecture, so this will need to be rerolled at some point.

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/Core/Entity/Tour.phpundefined
@@ -114,7 +123,9 @@ public function getTip($id) {
-    return array_keys($this->tips);
+    if (!empty($this->tips)) {
+      return array_keys($this->tips);

This should be removed.

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/tour/tip/TipPluginText.phpundefined
@@ -84,11 +85,52 @@ public function getAttributes() {
+  public function getExportProperties() {
+    $properties = parent::getExportProperties();

+++ b/core/modules/tour/lib/Drupal/tour/TipPluginBase.phpundefined
@@ -81,9 +95,86 @@ public function get($key) {
+  public function getExportProperties() {

This is disconcertingly close to the ConfigEntityBase method name. But this isn't ConfigEntityBase. Can we maybe pick another name?

+++ b/core/modules/tour/lib/Drupal/tour/TipPluginBase.phpundefined
@@ -51,28 +51,42 @@ public function __construct(array $configuration, $plugin_id, CacheDecorator $di
+  public function getId() {
+    return $this->get('id');
...
+  public function getPlugin() {
+    return $this->get('plugin');

If we're making wrappers for everything, why bother having get()? The purpose of get() was to avoid the need or usage of wrappers... We should either switch back to get() everywhere, or remove it completely.

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/Routing/TourUIController.phpundefined
@@ -0,0 +1,120 @@
+ * @todo.
+ */
+class TourUIController {

:)

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/Routing/TourUIController.phpundefined
@@ -0,0 +1,120 @@
+   * Provies a creation form for tour.
...
+   * Provies an edit form for a tour entity.
...
+   * Provies a creation form for a new tip to be added to a tour entity.

Provides

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/Routing/TourUIController.phpundefined
@@ -0,0 +1,120 @@
+   * Provies an edit form for a tour entity.
+   *
+   * @return array
...
+   * Provies a creation form for a new tip to be added to a tour entity.
+   *
+   * @return array
...
+   * Provies an edit form for tip to be updated against a tour entity.
+   *
+   * @return array
+   *   A renderable form array.
+   */
+  public function editTip(Tour $tour, $tip = '') {

Missing @param

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/Routing/TourUIController.phpundefined
@@ -0,0 +1,120 @@
+    if (empty($type)) {

I can't tell because there is no @param, but what data type would $type be here?

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/Routing/TourUIController.phpundefined
@@ -0,0 +1,120 @@
+    $request = drupal_container()->get('request');
...
+    $manager = drupal_container()->get('plugin.manager.tour.tip');

See #1938030: Replace View's usage of drupal_container->get() with Drupal::service(), we now have Drupal::service()

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/Routing/TourUIController.phpundefined
@@ -0,0 +1,120 @@
+    if (empty($tip) && empty($tour)) {

Once again, what data types are these? In some cases, you could do if (!$tip && !$tour) { since they're defined.

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/Tests/TourUITest.phpundefined
@@ -0,0 +1,183 @@
+ * Tests the Tour UI..

Two dots

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/TourDeleteForm.phpundefined
@@ -0,0 +1,74 @@
+  public function getForm(EntityInterface $tour) {
+    $this->entity = $tour;
+    return drupal_get_form($this);

This should use _form in the route, see #1934832: Provide a dedicated approach for using forms in routes

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/TourFormController.phpundefined
@@ -0,0 +1,251 @@
+  public function form(array $form, array &$form_state, EntityInterface $entity) {

This whole function could use more inline docs

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/TourFormController.phpundefined
@@ -0,0 +1,251 @@
+      // We default to the content language opposed to und (language not specified).

I don't think we say "we" in docs

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/TourFormController.phpundefined
@@ -0,0 +1,251 @@
+        // @todo, Setup forms on the tip types.

Is this a blocker or a follow-up? We need an issue if its a follow-up.

+++ b/core/modules/tour_ui/lib/Drupal/tour_ui/TourTipDeleteForm.phpundefined
@@ -0,0 +1,97 @@
+  public function getForm(EntityInterface $tour, $tip) {
+    $this->entity = $tour;
+    $this->tip = $tour->getTip($tip);
+    return drupal_get_form($this);

Do this in __construct, and use _form for the route

+++ b/core/modules/tour_ui/tour_ui.moduleundefined
@@ -0,0 +1,82 @@
+ * @todo.

:)

+++ b/core/modules/tour_ui/tour_ui.moduleundefined
@@ -0,0 +1,82 @@
+    'access callback' => TRUE,
...
+    'access callback' => TRUE,
...
+    'access callback' => TRUE,
...
+    'access callback' => TRUE,
...
+    'access callback' => TRUE,
...
+    'access callback' => TRUE,
...
+    'access callback' => TRUE,

+++ b/core/modules/tour_ui/tour_ui.routing.ymlundefined
@@ -0,0 +1,42 @@
+    _access: 'TRUE'
...
+    _access: 'TRUE'
...
+    _access: 'TRUE'
...
+    _access: 'TRUE'
...
+    _access: 'TRUE'
...
+    _access: 'TRUE'
...
+    _access: 'TRUE'

We likely need a hook_permission here :)

+++ b/core/modules/tour_ui/tour_ui.routing.ymlundefined
@@ -0,0 +1,42 @@
+tour_ui_listing:
+  pattern: '/admin/config/user-interface/tour'
+  defaults:
+    _controller: 'tour_ui.controller:listing'
+  requirements:
+    _access: 'TRUE'
+tour_ui_add:
+  pattern: '/admin/config/user-interface/tour/add'
+  defaults:
+    _controller: 'tour_ui.controller:add'
+  requirements:

Can you please put a blank line between routes? It helps for readability

I think there is a typo in the selection list. Currently the options are:

I guess this should be Bottom

I would like to have the possibility to enter a description for a tour. We are going to have a lot of tours, and not all of them will be UI tours where you can guess what they explain from the path to which they are attached.

I'm not sure if this belongs in the UI issue, but it was the UI that exposed this.

Feedback on the tip UI:
The attributes field group is kind of confusing I think. First time users will want to fill in both boxes, they would need to read the field label to understand they don't have to.

I think a select list that lets you choose the attribute type with next to it a text field for the actual attribute would be more intuitive.

For Walkthrough.it we've been using Step to name the elements a tour consists of, this is a more natural label for a broader set of use cases since it can give meaning to the order the tips are shown in. That way a tour could be used to:

• it could indicate a step in a sequence of actions a user needs to take
• as well the use primary use case Tour was built for: showing a sequence of tips on a site.

Also, none of the JS changes in this patch seem relevant at all. Please move them to a separate issue.

@tim, they are and I have a new issue for it #1938032: Simplify tour.js, see #15.

First: add a new tip to the views tour, do not fill "Data id" nor "Data class" and save. There are no validation errors.

Without JS changes:
- Visit a view, start the tour. There are only 10 items, the one you just added does not show up.

With JS changes:
- when the tour starts there are 11 items, the one you just added show up in a modal.

So it's either those JS changes or some kind of validation on the form. The modal thing is a joyride feature btw.

Well, we should have validation then. And have that other bit in the other issue, thanks for that.

Any feedback on #20 and #22?

As a naming issue, I guess #22 is not important enough to really start a big discussion. Unless of course someone else shares my opinion and wants to help push this forward.

...and now the tour_ui.form fails. Damn!

;)

#33: core-tour_ui-module-1921188-33.patch queued for re-testing.

Great work @nick_schuch
Please add configuration link admin/config/user-interface/tour to info.yml file.

+++ b/core/modules/tour/lib/Drupal/tour/TipPluginBase.phpundefined
@@ -72,4 +72,165 @@ public function get($key) {
+    ¶

Tabs/trailing whitespace

+++ b/core/modules/tour/lib/Drupal/tour/TipPluginBase.phpundefined
@@ -72,4 +72,165 @@ public function get($key) {
+   *   The form array. ¶

trailing whitespace

#39: 1921188-39-tour-ui.patch queued for re-testing.

Reroll of #39 with changes mentioned in 40-41. On second thought, should the trivial whitespace changes to the 'tour' module really be included here? I can bust out into a separate issue.

I'm doing manual testing on this at the Portland sprint.

Added ImageURL, ImageLocal, Youtube plugins to tour and tour_ui.

Path correction.

Adjusted whitespace issues

Yeah ... testbot is right ... I cannot apply any. I'll focus on the tour_ui and probably rip that code into tour_builder. Punishment will follow later :p

drush iq-apply-patch 1921188#52a --no-commit
drush iq-apply-patch 1921188 --no-commit

The patches from #52 are inter diffs. FWIW I cloned the sandbox,merged with the sandbox/8.x and tried to get it rolling but failed.

- Info file needs changes
- There is no list controller for tour,form

Attached is my effort done on core HEAD.
- I managed (by patching tour) to get a list of tours. (It's a bad patch I know but not at full)
- Add, Edit, etc fails
- I have no clue why tour is not providing for List/Form/etc as it provides the Entity. It does not feel right to let Tour UI provide for it. Changing the tour schema should be accompanied by changing the form and not rely on a contrib.

Tour UI needs more operations like clone, import, export to make building tours as easy as possible. Ie allow for textual exchange by tour editors who are by definition no coders but UX experts.

I thought enable/disable was in the active config files so I uncommented those which was wrong.

Let's please move the tour_ui to contrib asap so we can work on that. That probably makes https://drupal.org/project/tour_builder obsolete but the sooner the better :)

(my 2cents)

(doh) $ git diff --cached > ~/Downloads/1921188-tour_ui-58.txt

Related issue #2021361: tour should be in front of toolbar (z-index)

Tnx @nick_schuch

I've copied over the code from the core sandbox http://drupal.org/sandbox/tim.plunkett/1698392 to the contrib to be Tour UI sandbox https://drupal.org/sandbox/nick_schuch/1933602

It still needs work and I'm not sure whether core should fix #2025065: Tour module must add list, render and form controllers for Tour UI to work or Tour UI can fix it?

Further errors are
- Tour UI: #2025081: Add + Edit has LANGUAGE_ALL does not exists
- Tour UI: #2025085: Delete has notices

As Tour UI is now on it's own what should happen to

- #48 - #52: Added ImageURL, ImageLocal, Youtube plugins

@slewazimuth as Tour is now in core you probably create a separate issue for these?

Why are tips translatable? The complete tour has a language. Not it's tips right?

Patch from #63 makes tour UI list it's tours :)

Set to review to see testbot result.

I meant as per patch

My patch from #66 is same as #63 ... sorry for that. I still think Tips should not have language or translation.

The language issue is mentioned too in #1935120-11: Unusual language use in tour module thank to @kristen_pol (irc)

im5 using #66 and parrallel onto intermedia uses training link, though tour runs side by side. All plugins switched to your UI plugin model. Have updated local-image and added vimeo plugins. Intermedia is outside scope of tour capabilities, thus training nomenclature.

Latest patches (#66 == #63) gives exception

Undefined index: id	Notice	AnnotatedClassDiscovery.php	106	Drupal\Component\Plugin\Discovery\AnnotatedClassDiscovery->getDefinitions()

Update issue summary pointing to Tour UI

When I saw this marked fixed, I thought the whole Tour UI had gone in. Is postponed a better status?

The issue summary has a link to contrib sandbox Tour UI

Added link to Tour UI sandbox

So then it's won't fix?
It's not "fixed" if it wasn't committed to core.

As per #80

Updated issue summary.