Skip to content
This repository has been archived by the owner on Feb 17, 2021. It is now read-only.

Added container tour option, instead of always using document.body #313

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Benjamin-Dobell
Copy link
Contributor

@Benjamin-Dobell Benjamin-Dobell commented Jan 19, 2017

Closes #206

Alternate implementation of #197, as the most recent commits (those concerning position) seem to result in incorrect bubble positioning (wrong coordinate system). This implementation also supports specifying the container opt as either a string or a DOM element i.e. it functions the same as a step target.

@Benjamin-Dobell
Copy link
Contributor Author

Benjamin-Dobell commented Jan 19, 2017

Hmm, so after I go claiming the positioning is correct in this pull request it fails a positioning test. Whoops. Investigating...

@Benjamin-Dobell
Copy link
Contributor Author

There we go! All sorted.

Basically we can't reliably use getBoundingClientRect on the container element as it returns the rendered bounding box of the container's current content, if the container is not absolutely positioned (kind of crazy really). Meaning if all the children have margins (like in the test), the returned bounding box will encompass the margin-offset content, rather than the containers actual (left, top) viewport coordinates.

As such, I'm now resetting the bubble and then getting its bounding client rect, which will be the correct basis for the container's coordinate system. I've also added a fixedContainer option, which basically does the same job as a step's fixedTarget option i.e. Let's Hopscotch know that it shouldn't factor in scrolling offsets into its positioning when the container if fixed.

@suhmantha1
Copy link

@Benjamin-Dobell What's the status on getting this merged in? I'm looking to use this functionality for an overlay container

@levib
Copy link

levib commented Feb 1, 2018

@Benjamin-Dobell any luck here?

@Benjamin-Dobell
Copy link
Contributor Author

@suhmantha1 @levib I think you'll want to follow up with @zimmi88 as to why this wasn't merged (or closed). It conflicts now, but didn't at the time. I've been using this in production for 12 months without issue, but @zimmi88 may not be happy with the implementation.

@suhmantha1
Copy link

@Benjamin-Dobell I've replaced hopscotch.js with this PR version, but am getting the error Bubble rendering failed - template "bubble_default" is not a function.. How does your tour object look? Are you still starting tour like hopscotch.startTour(tourObject)?

@Benjamin-Dobell Benjamin-Dobell force-pushed the container branch 4 times, most recently from 04ac385 to 2f81e4f Compare February 19, 2018 15:39
@Benjamin-Dobell
Copy link
Contributor Author

Benjamin-Dobell commented Feb 19, 2018

I've just fixed a bug with the auto-scroll functionality and rebased on master so that this will merge cleanly.

The tests for this pull request (and pretty much every other PR) are failing because Hopscotch isn't using a lockfile - so the CI pulls down the wrong version dependencies. If you're looking to build/test this PR I'd suggest checking out #360 then cherry-picking this commit on top.

@suhmantha1 Yes, I'm still starting my tour with hopscotch.startTour(tourObject), tourObject looks something like:

{
  id: 'some-id',
  showPrevButton: true,
  bubbleWidth: bubbleWidth,
  steps: [
      {
          title: 'Hiya.',
          content: "Blah blah blah",
          target: someElement,
          placement: 'right',
          arrowOffset: bubbleWidth - 180,
          onNext: doSomeStuff
      },
      /* ... more steps */
  ],
  container: someDomElement
}

@dcantatore
Copy link

@linkedin Is this package dead?

@andi23rosca
Copy link

For anyone who doesn't want to bother with cloning the repo and going through the whole process Benjamin-Dobell recommended, i did that myself and attached a txt file containing the built code.

index.txt

@vasvir
Copy link

vasvir commented Sep 9, 2019

I just checked this PR and it works as advertised. I am able to show tours even when my main widget goes fullscreen. I had to manually apply the diff and I took the liberty of some minor edits (minimize diff, handle IE branch).

I am attaching the resulting patch and the resulting hopscotch.js and min.js

In my opinion the maintainer has to accept this pull request with minor modifications.

hopscotch.min.js.txt
hopscotch.js.txt
313v2.diff.txt

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fullscreen div: Parent of hopscotch other than the body
6 participants