Part of #1921152: META: Start providing tour tips for other core modules.

Problem/Motivation

Write tour integration for People admin page

Proposed resolution

Create tour yml files for required admin screens in admin/people.

Remaining tasks

Create patch for tour integration for primary People page

User interface changes

New tours

API changes

None

Technical pointers when creating tour tips

See: https://drupal.org/node/1921152#tour-tips-tech-note for tech notes on making tour tips.

Files: 
CommentFileSizeAuthor
#40 tour-people-2040845-40.patch2.39 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,013 pass(es). View
#40 interdiff.txt583 byteshussainweb
#37 interdiff-2040845-35-37.txt1.11 KBjorgegc
#37 tour-people-2040845-37.patch2.39 KBjorgegc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,997 pass(es). View
#35 tour-people-2040845-35.patch2.79 KBjorgegc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,032 pass(es). View
#26 interdiff.txt1.8 KBpameeela
#26 tour-people-2040845-26.patch2.58 KBpameeela
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,413 pass(es). View
#25 tour-people-2040845-25.interdiff.txt378 bytesnick_schuch
#25 tour-people-2040845-25.patch2.22 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 63,377 pass(es). View

Comments

lisarex’s picture

FileSize
1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 56,619 pass(es). View

Content written and edited by colleagues :)

lisarex’s picture

Status: Active » Needs review
FileSize
2.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,150 pass(es). View

Ignore previous. This patch contains two yml files

larowlan’s picture

Re the image, I don't think 'any site visitor' is true, I think 'approved users' would be more correct, as it's permission based.

nick_schuch’s picture

Issue tags: +Needs tests

Tour needs tests since we now have #2028535

floydm’s picture

FileSize
2.03 KB
3.93 KB
FAILED: [[SimpleTest]]: [MySQL] 58,844 pass(es), 1 fail(s), and 2 exception(s). View

Updated the patch to add UserAdminTourTest.php and fix a couple of whitespace issues in the yml file.

One of the tests fails because it can't find an element with class .action-links, which the second tip is attached to. However I can see such an item in the verbose testing output:

<nav class="action-links">
<li>
<a class="button button-action" href="/d8/admin/people/create">Add user</a>
</li>
</nav>

If I attach the second tour tip to class "button-action" the test passes. Does SimpleTest not handle the html 5 nav element?

Status: Needs review » Needs work

The last submitted patch, tour-people-2040845-5.patch, failed testing.

lisarex’s picture

Assigned: lisarex » Unassigned

unassigning myself

floydm’s picture

FileSize
2.37 KB
5.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es). View

Updated patch that adds the test for the /user/*/edit tour. It also modifies the user admin tour test to no longer test for nav class="action-links", which I mentioned in comment #5 I could not get to pass and which I suspect has to do with it being an html5 nav element. Only a guess though.

floydm’s picture

Status: Needs work » Needs review

Changing status to needs review.

floydm’s picture

FileSize
3.13 KB
2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,451 pass(es). View

Just noticed issue #2044399: Write tour integration for user edit page exists for the /user/*/edit tour and that that tour appears more complete, so I am rerolling this patch to remove the tour and test for /user/*/edit.

nielsonm’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Works for me. Marking this as RTBC.

batigolix’s picture

Status: Reviewed & tested by the community » Needs review

The code may work but the content & wording needs a proper review first

nielsonm’s picture

Assigned: Unassigned » nielsonm
Status: Needs review » Needs work
Issue tags: +Tour Content Review

Needs content review and new router system based update

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
3.09 KB
2.23 KB
FAILED: [[SimpleTest]]: [MySQL] 63,678 pass(es), 2 fail(s), and 0 exception(s). View

This patch updates the path to a route and updates the test coverage.

Status: Needs review » Needs work

The last submitted patch, 14: tour-people-2040845-14.patch, failed testing.

The last submitted patch, 14: tour-people-2040845-14.patch, failed testing.

nick_schuch’s picture

FileSize
389 bytes
2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 63,404 pass(es), 1 fail(s), and 0 exception(s). View

Fixes yml format issue.

nick_schuch’s picture

Status: Needs work » Needs review
nick_schuch’s picture

FileSize
2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 63,368 pass(es), 1 fail(s), and 0 exception(s). View
389 bytes

Reuploading for tests to run.

Status: Needs review » Needs work

The last submitted patch, 19: tour-people-2040845-17.patch, failed testing.

The last submitted patch, 17: tour-people-2040845-17.patch, failed testing.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tour-people-2040845-22.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 22: tour-people-2040845-22.patch, failed testing.

nielsonm’s picture

Assigned: nielsonm » Unassigned
nick_schuch’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 63,377 pass(es). View
378 bytes

Time for some green!

pameeela’s picture

FileSize
2.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,413 pass(es). View
1.8 KB

Updates content to better align with guidelines.

sun’s picture

This comment raises architectural design concerns and is not meant to discredit the work that went into this particular issue and patch.

+    label: 'Managing users'
+    body: 'View and edit user accounts.'
...
+    label: 'Add a user'
+    body: 'Create a new user account.'
...
+    label: 'Search for users'
+    body: 'Find users by applying filters.'
...
+    label: 'Update accounts'
+    body: 'Select one or more users via the checkboxes to apply changes to the accounts.'
...
+    label: 'Edit user account'
+    body: 'Make changes to one user account.'

Hrm. — Do we really want to explain every possible UI interaction in tour tips?

If we do, what kind of "tip" or explanation is "Create a new user account." meant to give that wouldn't be obvious by the UI already?

I was under the assumption we introduced Tour tips to explain non-obvious things in the user interface only?

→ If we need to provide hints for the most fundamental/basic operations, then our UI/UX must be seriously flawed? (which I don't think it is)

I'm afraid, this makes little sense to me. These kind of Tour tips will only make people avoid Tour module whenever possible.

Introducing any kind of such "tips" presents a major disrespect and discredit for the hard work of Drupal's Usability and Design teams, because the mere fact of pointing out the most basic user interface operations via Tour tips would mean that the entire user interface design is a utter #FAIL.

Or to put that in reverse, because it might not be obvious:

The user interface design is solely responsible for making obvious things obvious. User interface design enables end-users to use Drupal intuitively. A well-designed user interface needs absolutely zero explanation; every human immediately understands how to use it. That is the one and only goal of user interface design.

This patch (and likely others) introduces tips for the most obvious operations in the user interface. These operations should not need any kind of explanation in the first place. IF they do, then the solution is NOT to explain to the user how to use the interface. — The only acceptable solution is to fix the user interface design.

In short, if there is a problem with any of these basic operations, then we need to fix the UI. Tour tips should be reserved for non-obvious interactions.

A non-obvious interaction would be to point out that it is possible to add fields to users. Or to add view modes. Or that you can change the administrative user listing/view to customize it to your needs. Or that you can leverage the "Reset password" functionality to temporarily grant someone else access to a user account without sharing the password. — All of that is not obvious, and cannot be solved by user interface design.

batigolix’s picture

Some background discussion about the role of tours can be found in #1921152: META: Start providing tour tips for other core modules.

pameeela’s picture

sun, I could not possibly agree more. I think there has been some motivation to get more tours in and that has resulted in trying to create ones for a lot of admin pages that don't really need them. Or maybe this page should have one but as you said, it should point out the less obvious things.

There should probably be some more discussion about this sooner than later, to avoid unnecessary work.

rodrigoaguilera’s picture

Status: Needs review » Needs work

The routes section is separated in two lines.

the test need annotation to be detected as such instead of getInfo() and at least a setUp function and some actual tests.
Is as simple as the following example

  /**
   * {@inheritdoc}
   */
  protected function setUp() {
    parent::setUp();
    $this->adminUser = $this->drupalCreateUser(array('administer content translation', 'access tour'));
    $this->drupalLogin($this->adminUser);
  }

  /**
   * Tests Content translation tour tip availability.
   */
  public function testContentTranslationTour() {
    $this->drupalGet('admin/config/regional/content-language');
    $this->assertTourTips();
  }
rodrigoaguilera’s picture

Issue tags: +Novice

marking it as novice since is a task that can teach a lot about the drupal system

jorgegc’s picture

I am happy to work on this one guys.

jorgegc’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,032 pass(es). View

Uploading patch for review. Please let me know if there is anything else that we need to test. I've basically looked at what we are doing in Forum and replicated it.

benjy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/user/src/Tests/UserAdminTourTest.php
    @@ -0,0 +1,61 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function setUp() {
    +    parent::setUp();
    +    $this->adminUser = $this->drupalCreateUser(array('administer users', 'access tour'));
    +    $this->drupalLogin($this->adminUser);
    +  }
    

    Remove this, the user should be created in the parent using the permissions from $permissions.

  2. +++ b/core/modules/user/src/Tests/UserAdminTourTest.php
    @@ -0,0 +1,61 @@
    +  /**
    +   * Tests user list tour tip availability.
    +   */
    +  public function testUserListTour() {
    +    $this->drupalGet('admin/people');
    +    $this->assertTourTips();
    +  }
    

    This isn't needed, see TourTestBasic::testTips().

jorgegc’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,997 pass(es). View
1.11 KB

Changes as per comments in #36.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks great. RTBC if we're green.

cilefen’s picture

+++ b/core/modules/user/src/Tests/UserAdminTourTest.php
@@ -0,0 +1,42 @@
+   * The permissions required for a logged in user to test tour tips.

This is a total nit, but logged-in is a compound adjective and it should be hyphenated. You will find the wrong way in the same file as the right way in core, like ForumTest.

hussainweb’s picture

Issue tags: +DrupalSouth
FileSize
583 bytes
2.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,013 pass(es). View

I see http://english.stackexchange.com/a/11002 which also points to the same suggestion. Changing it. :)

I think this can still be RTBC.

jorgegc’s picture

Thanks @hussainweb :-)

webchick’s picture

Status: Reviewed & tested by the community » Postponed

Thanks a lot for your work on this issue! I reviewed it and it seems to work well.

I do have some concerns, however:

#1: The page in question is a View. In this respect it's identical to admin/content, admin/comments, etc. but those pages do not (yet) have Tours. Therefore, committing this patch would introduce an inconsistency with other, similar admin pages.

#2: I also share some of sun/pameela's concerns from #27/#29. We need to come to agreement on guidelines of when we do/do not use Tours for certain admin interactions.

Therefore, very sorry to do this, but I think we need to mark this postponed on figuring out such guidelines up in the parent issue at #1921152: META: Start providing tour tips for other core modules..

Since you've done great work on this and had the opportunity to play with Tour module, please chime in there with your thoughts!

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.