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 Appearance admin pages
Proposed resolution
Create tour yml files for required admin screens in admin/appearance.
Remaining tasks
Write tour integration for primary Appearance page
Decide on additional screens that require tour integration
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 |
---|---|---|---|
#76 | 2040375-76.patch | 3.73 KB | Gauravvvv |
#67 | interdiff_64-67.txt | 486 bytes | ridhimaabrol24 |
#67 | 2040375-67.patch | 4.94 KB | ridhimaabrol24 |
#64 | interdiff_62-64.txt | 1.15 KB | ridhimaabrol24 |
#64 | 2040375-64.patch | 4.94 KB | ridhimaabrol24 |
Comments
Comment #1
lisarex CreditAttribution: lisarex commentedPatch and screencast
http://screencast.com/t/5FJa9FEz2
Comment #2
lisarex CreditAttribution: lisarex commentedReady for review
Comment #3
larowlanMissing newline at end
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
These files don't belong in the patch - duplicates?
This file from another issue.
Comment #4
nick_schuch CreditAttribution: nick_schuch commentedTour needs tests since we now have #2028535: Provide a TourTestBase class for use by core and contrib modules
Comment #5
lisarex CreditAttribution: lisarex commentedGoing to unassign myself since I don't know how much time I'll have to get back into this, but I will try to make it happen!
Comment #6
xtfer CreditAttribution: xtfer commentedComment #7
xtfer CreditAttribution: xtfer commentedUpdated patch, providing the slightly reworked Appearance page tour, for review.
Still needs tests.
Comment #8
eojthebraveThis 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.
This file is missing a newline at the end of it.
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.
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 ..."
Comment #9
nick_schuch CreditAttribution: nick_schuch commentedHere 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.
Comment #11
nick_schuch CreditAttribution: nick_schuch commented9: drupal8.tour-module.2040375-9.patch queued for re-testing.
Comment #13
batigolix@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:
Do we need the
<p>
tags in the tip body? This results in the following markup: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."
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."
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))
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))
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?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"
Comment #14
batigolixComment #15
nielsonm CreditAttribution: nielsonm commentedComment #16
nielsonm CreditAttribution: nielsonm commentedComment #17
mkostrzewa CreditAttribution: mkostrzewa commentedComment #18
mkostrzewa CreditAttribution: mkostrzewa commentedComment #20
larowlan18: drupal8.tour-module.2040375-18.patch queued for re-testing.
Comment #22
wiifmRewrote 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.
Comment #23
wiifmThis patch is from the Drupal8 code sprint in New Zealand! #Drupal8NZ
Comment #24
Stefan Lehmann CreditAttribution: Stefan Lehmann commentedApplied patch and started the tour successfully, but the text for step 3 needs some more work in our opinion.
It says:
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.
Comment #25
dman CreditAttribution: dman commentedApplied 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:
If that's too long, sorry, but I'd rather suggest improvements...
Comment #26
wiifmThanks @Stefan Lehmann and @dman,
Attached is a new version that:
Comment #27
dman CreditAttribution: dman commentedLooks 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?
Comment #28
Stefan Lehmann CreditAttribution: Stefan Lehmann commentedThis patch here seems to introduce a new testing approach for Tours. Might be the way to go?
https://drupal.org/comment/8441507#comment-8441507
Comment #29
dman CreditAttribution: dman commentedI 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.
Comment #30
wiifmRe-rolled the patch, changes since last one:
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:
Hope this helps get an RTBC ;)
Comment #31
wiifmRe-roll.
Changes include:
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
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedRerolled Patch from #31
Comment #36
jhodgdonLooks like you forgot to set to Needs review. No, you didn't forget, but the issue didn't update correctly. ?!?
Comment #37
jhodgdonOh, I see you did it in a tag. Whoops.
I took a look at the patch, and have some concerns about the text:
a)
That is not really true. It is shown to *some* visitors on *some* pages.
b)
Should say "For example" or "For instance". As written it sounds like these are the only possible settings, which isn't true.
c)
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)
to -> from
e)
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".
Comment #38
webchickThis one seems like a good thing to do, but postponed on #1921152-109: META: Start providing tour tips for other core modules. for now.
Comment #39
mgiffordComment #45
cindytwilliams CreditAttribution: cindytwilliams commentedChanged the text per comment #37
Comment #46
cindytwilliams CreditAttribution: cindytwilliams commentedComment #48
DrupalMattS CreditAttribution: DrupalMattS at Maricopa Community Colleges commentedAfter 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.
Comment #49
DrupalMattS CreditAttribution: DrupalMattS at Maricopa Community Colleges commentedComment #50
DrupalMattS CreditAttribution: DrupalMattS at Maricopa Community Colleges commentedComment #52
cindytwilliams CreditAttribution: cindytwilliams commentedAs 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.
Comment #53
cindytwilliams CreditAttribution: cindytwilliams commentedComment #57
clemens.tolboomAs 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
Comment #61
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedComment #62
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedAdding test for the System Appearance Tour.
Comment #63
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedComment #64
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedFixing the patch for failed tests
Comment #66
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedComment #67
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedFixing failed tests.
Comment #68
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #69
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #75
smustgrave CreditAttribution: smustgrave at Mobomo commentedPatch 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.
Comment #76
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedAttached patch for 10.1.x please review
Comment #77
smustgrave CreditAttribution: smustgrave at Mobomo commented#76 appeared to have failures.
But wouldn't both as #3325445: [Policy] Remove tour module from core will postpone this.
Comment #79
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.