Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#94 | interdiff_90_94.txt | 546 bytes | joshua1234511 |
#94 | 2040845_94.patch | 4.14 KB | joshua1234511 |
#87 | 2040845.mov | 1.73 MB | gargsuchi |
#80 | 2040845_80.patch | 3.91 KB | vsujeetkumar |
#75 | interdiff_73-75.txt | 1.35 KB | vsujeetkumar |
Issue fork drupal-2040845
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
lisarex CreditAttribution: lisarex commentedContent written and edited by colleagues :)
Comment #2
lisarex CreditAttribution: lisarex commentedIgnore previous. This patch contains two yml files
Comment #3
larowlanRe the image, I don't think 'any site visitor' is true, I think 'approved users' would be more correct, as it's permission based.
Comment #4
nick_schuch CreditAttribution: nick_schuch commentedTour needs tests since we now have #2028535
Comment #5
floydm CreditAttribution: floydm commentedUpdated 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:
If I attach the second tour tip to class "button-action" the test passes. Does SimpleTest not handle the html 5 nav element?
Comment #7
lisarex CreditAttribution: lisarex commentedunassigning myself
Comment #8
floydm CreditAttribution: floydm commentedUpdated 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.
Comment #9
floydm CreditAttribution: floydm commentedChanging status to needs review.
Comment #10
floydm CreditAttribution: floydm commentedJust 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.
Comment #11
nielsonm CreditAttribution: nielsonm commentedWorks for me. Marking this as RTBC.
Comment #12
batigolixThe code may work but the content & wording needs a proper review first
Comment #13
nielsonm CreditAttribution: nielsonm commentedNeeds content review and new router system based update
Comment #14
nick_schuch CreditAttribution: nick_schuch commentedThis patch updates the path to a route and updates the test coverage.
Comment #17
nick_schuch CreditAttribution: nick_schuch commentedFixes yml format issue.
Comment #18
nick_schuch CreditAttribution: nick_schuch commentedComment #19
nick_schuch CreditAttribution: nick_schuch commentedReuploading for tests to run.
Comment #22
nick_schuch CreditAttribution: nick_schuch commentedComment #24
nielsonm CreditAttribution: nielsonm commentedComment #25
nick_schuch CreditAttribution: nick_schuch commentedTime for some green!
Comment #26
pameeela CreditAttribution: pameeela commentedUpdates content to better align with guidelines.
Comment #27
sunThis comment raises architectural design concerns and is not meant to discredit the work that went into this particular issue and patch.
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.
Comment #28
batigolixSome background discussion about the role of tours can be found in #1921152: META: Start providing tour tips for other core modules.
Comment #29
pameeela CreditAttribution: pameeela commentedsun, 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.
Comment #32
rodrigoaguileraThe 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
Comment #33
rodrigoaguileramarking it as novice since is a task that can teach a lot about the drupal system
Comment #34
jorgegc CreditAttribution: jorgegc commentedI am happy to work on this one guys.
Comment #35
jorgegc CreditAttribution: jorgegc commentedUploading 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.
Comment #36
benjy CreditAttribution: benjy commentedRemove this, the user should be created in the parent using the permissions from $permissions.
This isn't needed, see TourTestBasic::testTips().
Comment #37
jorgegc CreditAttribution: jorgegc commentedChanges as per comments in #36.
Comment #38
benjy CreditAttribution: benjy commentedLooks great. RTBC if we're green.
Comment #39
cilefen CreditAttribution: cilefen commentedThis 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.
Comment #40
hussainwebI see http://english.stackexchange.com/a/11002 which also points to the same suggestion. Changing it. :)
I think this can still be RTBC.
Comment #41
jorgegc CreditAttribution: jorgegc commentedThanks @hussainweb :-)
Comment #42
webchickThanks 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!
Comment #43
mgiffordComment #49
DrupalMattS CreditAttribution: DrupalMattS at Maricopa Community Colleges commented#Nashville2018 I am opening this issue in the DrupalCon Nashville Tour Mentored sprint
Comment #50
DrupalMattS CreditAttribution: DrupalMattS at Maricopa Community Colleges commentedBased on previous comment from Comment webchick I will mark back as postponed
Comment #51
DrupalMattS CreditAttribution: DrupalMattS at Maricopa Community Colleges commentedComment #52
grgcrlsn321 CreditAttribution: grgcrlsn321 as a volunteer commentedLooking into this.
Comment #53
grgcrlsn321 CreditAttribution: grgcrlsn321 as a volunteer commentedApplied patch and the tour for the People (admin/people) seems to already be working good. The tour goes through each button used on the page, unless there is need for more details I would consider this patch ready for commit.
Comment #54
grgcrlsn321 CreditAttribution: grgcrlsn321 as a volunteer commentedSetting this back to needs a review. The css is a little bit off and their isn't much description. The buttons pointed out seem pretty self explanatory. Maybe this tour is quite needed seeing that these steps our used throughout drupal admin pages.
Comment #56
John Cook CreditAttribution: John Cook at Creode commentedI've tested the patch from comment 40 but the "Tour" button did not appear after a new install of Drupal.
This should be in the optional config folder as the Tour module may not be enabled.
dependencies
section:This allowed the tour to be accessible.
Comment #57
clemens.tolboomI moved the tour to config/optional similar to views_ui and added dependencies as suggested in #56
I've seen that a lot using Tour UI so that is not a bug for this issue. It may be worth to submit a general issue but I guess `drush cach-rebuild` fixes it.
Comment #59
prashantgajare CreditAttribution: prashantgajare as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedFixing CS issues
Updated patch
Comment #60
prashantgajare CreditAttribution: prashantgajare as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedComment #62
prashantgajare CreditAttribution: prashantgajare as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedComment #64
vacho CreditAttribution: vacho at Skilld commentedSolving test problems over patch #59
Comment #65
volkswagenchickTagging for DrupalNorth 2019
Comment #68
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedMore Test Fixes. Please review.
Comment #70
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed Tests, Please review.
Comment #72
baroneThe patch failed to apply. Drupal 9.2-x.dev
Comment #73
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch created for 9.2.x.
Comment #75
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed the fail test.
Comment #76
clemens.tolboomI always wonder why patch contributors are removed from the list.
In this case
- prashantgajare
- vacho
- clemens.tolboom
AFAIK there is no tracking or reasoning for that.
Comment #78
Kristen PolThanks for the update.
1. Patch still applies cleanly to 9.3.
2. Manually tested as follows:
/admin/people
Tour
link in upper rightComment #79
Kristen PolHere's some feedback based on the above and reviewing the patch:
This step isn't triggering because the
button-action
class has been changed tobutton--action
. Perhaps usebutton--primary
to be more precise for this page?Nitpick: Consider simplifying to something like:
Not sure why this doesn't this catch the issue of step 2 not showing up.
Comment #80
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch given for 9.3.x.
Comment #81
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedAddressed #79.1 and 79.2, Please have a look.
Comment #84
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed the fail test, It is basically related to some deprecated property in config schema.
Comment #85
thejimbirch CreditAttribution: thejimbirch at Kanopi Studios commentedThe patch applies. I did get
fuzz
message that I have never seen before though:To test, I still had to import the config manually, since the user module was already enabled. Adding a tour to core will only show up for new users. Should there be an update hook to add the tour to existing sites?
Leaving this as Needs Review to answer that question, but the current patch works as expected.
Here's a screen recording of the tour.
Comment #87
gargsuchi CreditAttribution: gargsuchi at Salsa Digital commentedManually tested as follows:
Comment #89
Kristen PolThanks for the updated patch and the testing. Removing the novice tag as I don't think it applies anymore. From #85:
It should show up for existing sites, so back to needs work for that at least. Additionally, some minor nitpicks below:
Unrelated to issue so shouldn't be in patch.
Same as above.
Extreme Nitpick: Not sure if it's customary to make these alphabetical.
Comment #90
joshua1234511Updated the patch with a update_hook to make it work with existing sites and not having to reinstall the module.
Included the Nitpick/ code review suggestions from #89
Comment #92
Kristen PolThanks for the update @joshua1234511.
Is this code similar to other code in core? I was hoping to compare it, if so.
Nitpick: Reword to something like: Import people tour configuration.
Comment #93
joshua1234511Would need to update this as it has missed the window for 9.3. Therefor change the name to system_update_9401()
Comment #94
joshua1234511Updated the patch with the nitpick and hook update support for v9.4.x
The code for hook update import YML referenced form
web/core/modules/config/src/Form/ConfigSingleImportForm.php
web/core/lib/Drupal/Core/Config/ConfigInstaller.php
Comment #96
clemens.tolboomThe
seems unique (or we have still to few tours added to core) in (re)loading the tour.
I vaguely remember a discussion about updates should not update tours. Cannot find it :-/ Maybe it was Tour UI #3214357: Will changes from (core 9.2+) Tour hurt? ?
Comment #97
thejimbirch CreditAttribution: thejimbirch at Kanopi Studios commentedAccording to the comment in #89,
Comment #98
clemens.tolboomComment #102
quietone CreditAttribution: quietone at PreviousNext commentedThis 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.
Comment #103
quietone CreditAttribution: quietone at PreviousNext commented