Write tour integration for Appearance pages

+++ b/core/modules/system/config/tour.tour.appearance-update.ymlundefined
@@ -0,0 +1,13 @@
\ No newline at end of file

+++ b/core/modules/system/config/tour.tour.appearance.ymlundefined
@@ -0,0 +1,45 @@
\ No newline at end of file

+++ b/core/modules/system/tour.tour.appearance-update.ymlundefined
@@ -0,0 +1,13 @@
\ No newline at end of file

Missing newline at end

+++ b/core/modules/system/config/tour.tour.appearance.ymlundefined
@@ -0,0 +1,45 @@
+    body: '<p>The default theme is shown on all user-facing pages.</p>'

I think we should link to d.o themes page saying. 'You can download additional contributed themes to change the appearance of your site from Drupal.org' or similar

+++ b/core/modules/system/config/tour.tour.appearance.ymlundefined
--- /dev/null
+++ b/core/modules/system/tour.tour.appearance-update.ymlundefined

+++ b/core/modules/system/tour.tour.appearance-update.ymlundefined
--- /dev/null
+++ b/core/modules/system/tour.tour.appearance.ymlundefined

These files don't belong in the patch - duplicates?

+++ b/core/modules/system/tour.tour.appearance.ymlundefined
--- /dev/null
+++ b/core/modules/toolbar/config/tour.tour.toolbar.ymlundefined

This file from another issue.

Tour needs tests since we now have #2028535: Provide a TourTestBase class for use by core and contrib modules

Updated patch, providing the slightly reworked Appearance page tour, for review.

Still needs tests.

This is my first time reviewing a tour, so if there are guidelines somewhere for writing one of these and/or what they should contain I would love to know. :)

Otherwise, here's my feedback after an initial pass.

And this still needs tests.

diff --git a/core/modules/system/config/tour.tour.appearance-update.yml b/core/modules/system/config/tour.tour.appearance-update.yml
new file mode 100644
index 0000000..74b536f

index 0000000..74b536f
--- /dev/null

--- /dev/null
+++ b/core/modules/system/config/tour.tour.appearance-update.ymlundefined

This file is missing a newline at the end of it.

+++ b/core/modules/system/config/tour.tour.appearance.ymlundefined
@@ -0,0 +1,53 @@
+      data-class: 'theme-default .operations .theme-settings'
+    body: '<p>Edit the theme''s colors, presence of certain page elements, logo and favicon.</p>'
+  disabled-themes:

There is an extra ' here, and I think this should read something like:

"Many themes have additional settings, edit a theme's settings to toggle the presence of certain page elements, change the logo, or add a favicon. Some themes even support changing colors."

Since not all themes support color module.

+++ b/core/modules/system/config/tour.tour.appearance.ymlundefined
@@ -0,0 +1,53 @@
+      data-class: 'system-themes-list-disabled .theme-info:first .enable-theme-default'
+    body: '<p>Change your theme by enabling a new theme, and setting it as the default.</p><p>You can download additional contributed themes to change the appearance of your site from <a href="https://drupal.org/project/themes">Drupal.org</a>.</p>'
+  admin-theme:

I think this might be clearer is we said "Change the appearance of your site by enabling a new theme and setting it as the default ..."

Here we go. There were quite a number of changes in this reroll due to HEAD moving on. I have also incorporated the changes from #8.

9: drupal8.tour-module.2040375-9.patch queued for re-testing.

@eojthebrave: there are:
- docs for writing tours: https://drupal.org/developing/api/tour
- style guidelines for tours: https://drupal.org/node/2040099
- interface patterns for tours: https://drupal.org/node/2000088 (don't really see the point of this)
All of them are under construction and will be improved while writing tours.

The duplicate single quote is to escape a single quote?. Can someone confirm this? I tried a backslash as escape characters but it doesn't work.

Some feedback regarding the tour in patch #9:

body: '<p>Site appearance is controlled by themes. Themes control the design, layout, structure, and other visual elements.</p>'

Do we need the <p> tags in the tip body? This results in the following markup:

<p class="tour-tip-body" id="tour-tip-page-overview-contents"><p>Site appearance is controlled by themes. Themes control the design, layout, structure, and other visual elements.</p></p>

I think the paragraph marks should be removed from all tips. On the other hand. What markup can you use if you want paragraphs inside a tip?

Regarding the same tip. It is not a very nice sentence. How about: " Your site's appearance is controlled by themes. They control the design, layout, structure, and other visual elements."

The default theme is shown on all user-facing pages

To prevent using the jargon-ish "user-facing pages" this tip could say: "The default theme is the theme that is shown to the visitors of the website."

Some themes even support changing colors

This sounds like color configuration is something really special (yes it was in the days of Drupal 4.5). More neutral would be: "Some themes allow the administrator to configure a theme’s color scheme using a JavaScript-based color picker." (I'm rephrasing from the book Using Drupal 7 (O'reilly))

The administrative theme has a different look and layout to the default theme, and is used only on site administration pages.

It is good to repeat the term "admin theme" because it is used in the UI and quickly explain why it is useful to have different back and front end themes: for example like "The admin theme is the theme that will be used for all administration pages (starting with "admin" in the URL path). It helps you to recognize that you are in the back-end of the site." (again rephrasing from Using Drupal 7 (O'reilly))

  disabled-themes:
    id: disabled-themes
    attributes:
      data-class: 'system-themes-list-disabled'

When all themes are enabled this tip is not shown. The class system-themes-list-disabled doesn't exist then. But the tip contains information that is still useful even with no disabled themes: the fact that you can set a theme as default. So this tip should be attached to an element that is always present?

You can choose a new admin theme here

I feel we should prevent using "here, there, this" and other textual pointers, because the tip with its arrow already visually points to something. How about: "Choose the theme that will be used for all administration pages and optionally for content creation and editing pages as well". (quoting Using Drupal 7 (O'reilly))

About the tip labels: I think we should use verbs in the -ing form, as we do elsewhere in Drupal docs. So: " Changing the theme" and "Setting the admin theme"

• Fixed all the issues listed in comment: https://drupal.org/comment/8480903#comment-8480903

18: drupal8.tour-module.2040375-18.patch queued for re-testing.

Rewrote most of the tour YAML so it matches the views_ui tour YAML, made the weights into integers (so hopefully the tests pass now). No string changes.

This patch is from the Drupal8 code sprint in New Zealand! #Drupal8NZ

Applied patch and started the tour successfully, but the text for step 3 needs some more work in our opinion.

It says:

Some themes allow the administrator to configure a theme's color scheme using a JavaScript-based color picker.

But this is misleading, as you normally have much more options here, like show / hide different standard page elements like logo, breadcrumb etc. This should be picked up too.

Applied patch OK.
I was not sure about the desired outcome from the issue description, but can confirm that:
* after resetting come caches (actually uninstalled and re-installed tour.module
* visited sites /admin/appearance
* saw new 'Tour' option in the toolbar top right
* clicking it started tour
* I agree the step 3 re color-picker is both too vague and too specific at the same time. Better text needed.
* I'd add that the order of steps is not great. Place the explanation of what admin theme is next to the option to enable it. Currently step 5 breaks the flow

Suggested replacement text (strawman)
Before:

Theme settings

Some themes allow the administrator to configure a theme's color scheme using a JavaScript-based color picker.

Theme settings

Different themes provide different configurable options. Some allow you to choose your own colors, some have settings specific to responsive (mobile) layouts, and others allow you to choose different style variations

If that's too long, sorry, but I'd rather suggest improvements...

Thanks @Stefan Lehmann and @dman,

Attached is a new version that:

• Uses dman's new wording for the theme settings description
• Re-orders tour item 4 and 5 for better flow.

Looks good to me.
Needed to rebuild my site again (getting sick of this with D8) for it to show up right, but I'm seeing the updated text, and the better flow working.

This patch includes a test framework file ... but no actual tests?
I'm guessing we can't go RTBC without a test of some sort. Anyone have an example test for tour.module to start from?

This patch here seems to introduce a new testing approach for Tours. Might be the way to go?

https://drupal.org/comment/8441507#comment-8441507

I like that that approach tests whether the data-id or class element is on the page - as that's the thing I feel most likely to break silently.
It doesn't test the *behavior* but as that's all client-side JS, I don't imagine that's possible without browser-session emulation - which is out of scope with web test cases.

I'd like to see that If that ExtendTourTest framework could actually be put into TourTestBasic, as it should be able to test all valid tours generically - so yeah, lets follow that for a bit.
As with all testing, it's the setUp() that is scary at first. Once someone else has laid the base for that though, each functional test is easy.

Re-rolled the patch, changes since last one:

• Fixed typo in getInfo() for the word appearance.
• Moved and renamed the test file so it makes more sense.

Have confirmed that the tests do indeed work, my command to verify this

sudo -u www-data php core/scripts/run-tests.sh --verbose --url http://d8.localhost/ --file core/modules/system/lib/Drupal/system/Tests/Tour/AppearanceTourTest.php

and the output:

Drupal test run
---------------

Tests to be run:
  - Drupal\system\Tests\Tour\AppearanceTourTest

Test run started:
  Sunday, April 6, 2014 - 11:04

Test summary
------------

Drupal\system\Tests\Tour\AppearanceTourTest                   17 passes                            3 messages

Test run duration: 49 sec

Detailed test results
---------------------


---- Drupal\system\Tests\Tour\AppearanceTourTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Pass      Other      TourTestBasic.php   48 Drupal\tour\Tests\TourTestBasic->se
    Enabled modules: tour
Pass      Role       TourTestBasic.php   64 Drupal\tour\Tests\TourTestBasic->se
    Created role ID 'i6gz6u0e' with name 'E]UfC~y|'.
Pass      Role       TourTestBasic.php   64 Drupal\tour\Tests\TourTestBasic->se
    Created permissions: administer themes, access tour, view the administration
    theme
Pass      User login TourTestBasic.php   64 Drupal\tour\Tests\TourTestBasic->se
    User created with name hXZaiIos and pass r4FiW5k45D
Pass      Browser    TourTestBasic.php   65 Drupal\tour\Tests\TourTestBasic->se
    GET http://d8.localhost/user returned 200 (3.7 KB).
Pass      Browser    TourTestBasic.php   65 Drupal\tour\Tests\TourTestBasic->se
    Valid HTML found on "http://d8.localhost/user"
Pass      Browser    TourTestBasic.php   65 Drupal\tour\Tests\TourTestBasic->se
    GET http://d8.localhost/user/2 returned 200 (3.98 KB).
Pass      Browser    TourTestBasic.php   65 Drupal\tour\Tests\TourTestBasic->se
    Valid HTML found on "http://d8.localhost/user/2"
Pass      User login TourTestBasic.php   65 Drupal\tour\Tests\TourTestBasic->se
    User hXZaiIos successfully logged in.
Pass      Browser    TourTestBasic.php   73 Drupal\tour\Tests\TourTestBasic->te
    GET http://d8.localhost/admin/appearance returned 200 (14.3 KB).
Pass      Browser    TourTestBasic.php   73 Drupal\tour\Tests\TourTestBasic->te
    Valid HTML found on "http://d8.localhost/admin/appearance"
Pass      Other      TourTestBasic.php   74 Drupal\tour\Tests\TourTestBasic->te
    Found corresponding page element for tour tip with class .theme-default
    .theme-info
Pass      Other      TourTestBasic.php   74 Drupal\tour\Tests\TourTestBasic->te
    Found corresponding page element for tour tip with class .operations
    .theme-settings
Pass      Other      TourTestBasic.php   74 Drupal\tour\Tests\TourTestBasic->te
    Found corresponding page element for tour tip with class .operations
Pass      Other      TourTestBasic.php   74 Drupal\tour\Tests\TourTestBasic->te
    Found corresponding page element for tour tip with class .theme-admin
    .theme-info
Pass      Other      TourTestBasic.php   74 Drupal\tour\Tests\TourTestBasic->te
    Found corresponding page element for tour tip with class
    .system-themes-admin-form
Pass      Other      TourTestBasic.php   74 Drupal\tour\Tests\TourTestBasic->te
    Total 6 Tips tested of which 1 modal(s).

Hope this helps get an RTBC ;)

Re-roll.

Changes include:

• Moving files due to PSR4

Tests pass with:
sudo -u www-data php core/scripts/run-tests.sh --verbose --url http://d8.localhost/ --file core/modules/system/src/Tests/Tour/AppearanceTourTest.php

Looks like you forgot to set to Needs review. No, you didn't forget, but the issue didn't update correctly. ?!?

Oh, I see you did it in a tag. Whoops.

I took a look at the patch, and have some concerns about the text:

a)

+    body: 'The default theme is the theme that is shown to the visitors of the website.'

That is not really true. It is shown to *some* visitors on *some* pages.

b)

+    body: "Different themes provide different configurable options. Some allow you to choose your own colors, some have settings specific to responsive (mobile) layouts, and others allow you to choose different style variations."

Should say "For example" or "For instance". As written it sounds like these are the only possible settings, which isn't true.

c)

Change the appearance of your site by enabling a new theme and setting it as the default. You can download additional contributed themes to change the appearance of your site from <a href="https://drupal.org/project/themes">Drupal.org</a>.

The URL should be https://www.drupal.org/project/project_theme
Also the wording here... I don't think we need another "change the appearance of your site" in the second sentence? You already explained in the first sentence that this is what enabling a theme does.

d)

The admin theme has a different look and layout to the default theme, 

to -> from

e)

Choose the theme that will be used for all administration pages and optionally for content creation and editing pages as well.

This directly contradicts what the previous item says: " used only on site administration pages.". Fix the previous bullet; this one is right. Also you can omit "as well".

This one seems like a good thing to do, but postponed on #1921152-109: META: Start providing tour tips for other core modules. for now.

Changed the text per comment #37

After reviewing at #nashvill2018, i moved the yml file to tour as appearance cant be re-installed to process the installation. I also updated some classes and text to be more user friendly.

As noted in the comments at #1921152-146: META: Start providing tour tips for other core modules, I moved the yml file to the tour/config/optional directory.

As the tour belongs to a system route it belong to system so moved it to options.

A also added the testbot complain about dependencies section.

This needs a test similar to core/modules/views_ui/tests/src/Functional/ViewsUITourTest.php

Adding test for the System Appearance Tour.

Fixing the patch for failed tests

Fixing failed tests.

Patch or MR doesn't apply anymore
The last patch or MR doesn't apply to the target branch, please reroll the code so that it can be reviewed by the automated testbot.

Attached patch for 10.1.x please review

#76 appeared to have failures.

But wouldn't both as #3325445: [Policy] Remove tour module from core will postpone this.

This extension is being deprecated, see #3336033: [Meta] Tasks to deprecate Tour module. It will be removed from core and moved to a contrib project, #3376099: [11.x] [Meta] Tasks to remove Tour.

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

Will be part of 2.0.x branch.