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.Updated: Comment #10
Problem/Motivation
The tour module does not respond to changes on the page. Here, there's a popup which looks good:
Yet, when the toolbar is opened or when the sidebar moves from left to right, the layout breaks:
Proposed resolution
Added a method to refresh the position of the tour and trigger the refresh once the viewport offsets change.
Remaining tasks
Decide how to fix, see #18
User interface changes
- The popup's position should change according to the events that trigger changes in the layout.
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #36 | After patch.png | 114.24 KB | DishaKatariya |
| #36 | Before Patch-.png | 107.18 KB | DishaKatariya |
| #34 | After-Patch-2224541-33.png | 363.87 KB | Manibharathi E R |
| #34 | Before-Patch-2224541-33.png | 266.04 KB | Manibharathi E R |
Issue fork drupal-2224541
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:
changes
Comments
Comment #1
sqndr CreditAttribution: sqndr commentedComment #2
wwwalker CreditAttribution: wwwalker commentedThe bug occurs when the menu is changed from horizontal to vertical orientation by clicking on the 'vertical orientation' button on the top right of the browser. But the error does not occur when the menu is changed by resizing the browser, i.e. menu does not overlap the tip box and tip points to the right HTML tag. If the menu is changed from vertical to horizontal by clicking 'horizontal orientation' button, the tip box is pointing to the wrong position and is not following tag until the browser is resized when it points to right tag again. It seems that a side effect of tour.js (Backbone) is not updating its tip position until browser is resized.
Comment #3
paulh CreditAttribution: paulh commentedPossibly a solution is to position the Tours from the corner of the page element, not the corner of the body.
Comment #4
Sam152 CreditAttribution: Sam152 commentedAdded a method to refresh the position of the tour and trigger the refresh once the viewport offsets change.
Comment #5
fenstratGood work @Sam152. The
$tour.joyride('is_phone')seems odd, but it is how joyride does it. Tested and this fixes the issue nicely.Just a minor nitpick:
Missing space between else and {
Comment #6
Sam152 CreditAttribution: Sam152 commentedThanks for the feedback fenstrat. Fix attached.
I think it's pretty annoying that joyride doesn't provide a method that abstracts this tidbit away. I had considered triggering resize.joyride, but it seemed hackier than the minor duplication.
Comment #7
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedRerolled patch.
Comment #10
fenstratThis is good to go. There hasn't been any movement on #2027623: De-fork jQuery Joyride and update to latest stable release (and at a glance it looks like joyride 2.1 will not fix this). This patch fixes a real bug we have, so this is RTBC.
Comment #11
alexpottTagging for a JS maintainer review.
Comment #12
nod_This patch makes the JS crash with the error
Uncaught TypeError: Cannot read property 'width' of undefinedsome things are not initialized properly in joyride in those conditions. Putting aif ($tour.length) {}helps on most pages but still breaks other pages. The newrefreshPositionmethod needs to check if a tour is open before doing anything.The event needs to be namespaced, bind it to
drupalViewportOffsetChange.joyrideplease.Comment #13
Sam152 CreditAttribution: Sam152 commentedThanks for the review nod_. Binding to ".joyride" causes the fix to only work once, as joyride destroys all of the events when you close it for the first time, causing subsequent reopens to break. Instead I've namespaced to "tour". Also fixed the inactivty by using the 'isActive' flag.
Comment #14
Sam152 CreditAttribution: Sam152 commentedComment #15
nod_can you go with this.refreshPosition.bind(this) ? don't need underscore to do that it's native.
Comment #16
Sam152 CreditAttribution: Sam152 commentedSaw that pattern a few other places in core. It saves us from having to go
var _this = this; function() { _this.refreshPosition.bind(_this) }, so it is a fair bit more concise.Comment #17
Sam152 CreditAttribution: Sam152 commentedMisunderstood #16, cleared up on IRC. New patch attached.
Comment #18
nod_The position when playing around with the toolbar isn't really perfect. Maybe a timing thing. When you have a tip pointing at an element opening and closing the menu tray makes the tip point to something irrelevant.
Not sure how to fix.
Comment #30
quietone CreditAttribution: quietone at PreviousNext commentedThis is reproducible on Drupal 9.5.x. The patch does not apply so I was not able to test it. It could be re-rolled but then
from @nod_ in #18
So, it seems the next step is to determine how to fix this or possibly close as works as designed. Setting the status to NR for discussion.
Comment #31
finnsky CreditAttribution: finnsky at Skilld commentedComment #33
andypostpatches are hidden because issue using MR
Comment #34
Manibharathi E R CreditAttribution: Manibharathi E R for Srijan | A Material+ Company commented#33 Patch Tested and Applied successfully on Drupal 9.5.x.
Steps to Reproduce:
1. Install using standard install profile
2. Enable the Tour module
3. Go to structure->views->Edit the Content View
4. Start the Tour
5. Change the Toolbar orientation Vertical.
6. Apply the Patch and check the UI.
Comment #35
DishaKatariya CreditAttribution: DishaKatariya at QED42 for Drupal India Association commentedComment #36
DishaKatariya CreditAttribution: DishaKatariya at QED42 for Drupal India Association commentedVerified and tested patch #33 applied successfully and looks good to me on Dupal 9.5.x version
Testing steps-
1. Install the module
2. Enable the Tour module
3. Go to structure->views->Edit the Content View
4. Start the Tour
5. Change the Toolbar orientation Vertical.
6. Apply the Patch and check the UI.
7. Check the tooltip/popup is getting cropped
Testing results-
The popup is appearing fine now and not getting cropped
Attaching the screenshot for reference:
Can be moved to RTBC
Comment #37
alexpottWe'll need a D10 version of the patch as there are no .es6 files there and JS standards have changed because we don't support IE.
Also can we add a test for this in \Drupal\Tests\tour\FunctionalJavascript\TourJavascriptTest::testGeneralTourUse() or maybe a new Nightwatch test?
Comment #40
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.