Problem/Motivation
As discussed in #1921152: META: Start providing tour tips for other core modules. we should add a block that shows the Available Tours. Availability depends on whether the module they belong to is enabled.
This block should be added by default to the main help page (admin/help). This screenshot visualizes what this should look like:
Proposed resolution
Create Available Tours block
Remaining tasks
- create block. Some work on this was already done in https://drupal.org/comment/7964451#comment-7964451
- create test
- enable block by default (in standard profile?)
User interface changes
- the main help page (admin/help) will be affected
API changes
No api changes
Comment | File | Size | Author |
---|---|---|---|
#22 | tour-block-2205801-22.patch | 10.46 KB | Antti J. Salminen |
#19 | tour-block-2205801-19.patch | 10.53 KB | Antti J. Salminen |
#10 | tour-block-2205801-10.patch | 19.71 KB | clemens.tolboom |
#10 | tour-block-2205801-interdiff-7-10.txt | 1.06 KB | clemens.tolboom |
#8 | tour-block-2205801-7.patch | 34.76 KB | clemens.tolboom |
Comments
Comment #1
clemens.tolboomI'll take this
Comment #2
larowlani'd been working on this over in #1921152: META: Start providing tour tips for other core modules., here's the wip patch
needs tests
we can unit test both the tour manager and the block I'm pretty sure.
Comment #4
larowlanNote the patch above includes a content-types tour, which should be removed, as well as some js changes, which also need reverting.
Comment #5
batigolixThis is the string I used for the introduction sentence:
Comment #6
clemens.tolboomText from #5 needs to be placed here
Comment #7
clemens.tolboomRemoved
Extended hook_help and added tour_help_addition(). Adding text from #5
Note the text from tour_help_addition appear to early on http://drupal.d8/admin/help but looks fine to me on http://drupal.d8/admin/help/tour
It's a little schizo now tour help appear on so many pages.
Notice: Undefined index: title in template_preprocess_links() (line 1296 of core/includes/theme.inc).
which is caused by the Tour toolbar button.Comment #8
clemens.tolboomThe failure
Notice: Undefined index: title in template_preprocess_links() (line 1296 of core/includes/theme.inc).
is caused byof which it's structure is used both for the block and now new tour_help_addition() and menu building (failing) AFAIK.
(and with patch)
Comment #10
clemens.tolboomI've fixed both invalid schema error and undefined index. Let's see all pass.
New findings
Comment #11
jhodgdonThere are a few things I don't understand about this patch:
a) I don't understand the block/hook_help() strategy. In the block config, there is:
But then in hook_help, there is:
This seems like it would be adding the tour list information twice?
b) And if you are doing that in hook_help(), why do you need a block at all?
c)
How is just adding the route parameters to an array going to cause an exception here? This just seems like a weird try/catch.
Comment #12
larowlanYes, the block on its own should be enough. Perhaps we could make the intro text configurable in the block. Failing that I think it belongs in the block build method, instead of hook help.
Re the try catch, it is possible to define a tour using a route name with no parameters, eg views does this so the tour displays when editing any view. However in order to show a sample link, we need a concrete param. That is the purpose of the alter hook and the views implementation. If you call the generateFromRoute method with a route name that includes placeholders in its pattern/path, but don't provide param values for these placeholders, that exception is thrown.
Comment #13
jhodgdonRegarding the try/catch, it is just around code that calls array(), not around code that calls generateFromRoute(), as far as I can see?
Comment #14
clemens.tolboomAs I said in #7-5 about the block it lists available tours everywhere if configured properly.
I don't think we should rely on the block configuration to get the lists of tours on the help pages.
Another scenario possible when Tour UI is fixed is site builders add tours. My guess is we need views to list tour.
So the block in _not_ enough :)
Comment #15
jhodgdonThat is fine that we need a tour block and the hook_help().
What isn't fine is that the config shows the block being put on the help page, which is redundant with the hook_help(). So either the config isn't actually working, or hook_help() code isn't working, or the tours would appear twice, right?
I actually think that the hook_help() code that appears to be trying to put the tours list on the main help page is probably what is not working. This should be done in the Help module where it builds the help page. It could just say basically "if the tour module is enabled, call this function and make a list of tours", right after it makes the list of help topics. To me, that seems like the right place to do it, rather than trying to do it in hook_help(), which for individual paths, normally adds text to the top of the page anyway.
Comment #16
clemens.tolboomI'm not sure I've time next week so unassigning
Comment #17
clemens.tolboomShould we consider #2069073: Allow Tours to be taken by users that cannot access the Toolbar (e.g. anonymous users) too?
Comment #18
Antti J. Salminen CreditAttribution: Antti J. Salminen commentedWorking on this.
Comment #19
Antti J. Salminen CreditAttribution: Antti J. Salminen commentedHere's a reroll to make this work with latest HEAD. Removed the leftover bits from the base patch and the toolbar item. I also removed the the tour permission from this one because to me it seemed likely to belong in a separate issue and wanted to limit the scope at first anyway.
This patch only adds the block to admin/help and removes that part of hook_help. In my opinion it would be best to only have the block and in any case not have both. Implementing it via hook_help would still allow placing it on the help page but since that is meant for contextual help text on a route, it doesn't really seem to fit. Should the block placement configuration be in the module or in the standard install profile?
The patch was missing the hook implementation for views_ui so I added that back. As jhodgdon pointed out, the try/catch block doesn't work and I was hit with the exception it was attempting to catch before adding the hook. The hook may still need to be changed to find an enabled view instead of relying on the default one for front page.
Marking "Needs review" just to run the tests.
Edit: Seems I forgot to remove the UUID in the block config file so that should also be done.
Comment #20
jhodgdonPatch is still "needs work"; see previous comments.
Comment #22
Antti J. Salminen CreditAttribution: Antti J. Salminen commentedFixed the block config so this should actually pass the tests now. That's the only change, still just triggering tests and needs work.
Comment #23
Antti J. Salminen CreditAttribution: Antti J. Salminen commentedComment #24
jhodgdonFor the contrib (currently sandbox) Configurable Help module (which creates a Help Topic config entity), I took a different approach, but I ended up with somewhat similar code.
In Config Help, the module takes over the admin/help page (with a route alter) and builds it with a hook (which could be a plugin). The hook allows modules to add sections to the help page, and I implemented the hook on behalf of the core Help module, the Config Help module, and the Tour module, to make three sections on the page, rather than making a block.
One thing I ran into is that there are currently a few tours in Core. The one in the Views UI module is active when you're on a view edit page. But you cannot make a direct link to this page, because it is editing a particular view and requires a View ID parameter to the route.
So I ended up with code like this:
Just thought I'd post it here if it's helpful... I'm sure you'll run into the same thing.
In general... I also am not sure if a block is really the right solution here. You might be better off using a page build alter or something like that, so it gets added to the main content area of the page?
Comment #25
jhodgdonI've spun off part of the previously mentioned sandbox module to #2661200: Make admin/help page more flexible, and list tours on it. It would take care of the needs of this issue. I should have a Core patch shortly, since I already have it working in the Sandbox with tests and everything. The sandbox is at https://www.drupal.org/sandbox/jhodgdon/2369943 if interested.
Comment #26
Wim Leers#2661200: Make admin/help page more flexible, and list tours on it has landed. AFAICT that means this issue is already fixed there?
Comment #27
jhodgdonTechnically that didn't make a Block for tours. But it did accomplish the goal of adding Tours to the admin/help page.
So the question is whether the Tour module maintainers want there to be an Available Tours block for other purposes... the main stated goal of this issue is to get Tours on the Help page, though, and yes that is now done (in 8.1.x and 8.2.x).
Comment #28
jhodgdonI think I will go ahead and close this as a duplicate of the other issue, since the main goal of "List tours on admin/help" has been satisfied now (in 8.1).
Comment #29
larowlan+1