Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements #28

Merged
merged 30 commits into from
Jul 22, 2015
Merged

Improvements #28

merged 30 commits into from
Jul 22, 2015

Conversation

yhnavein
Copy link
Contributor

@yhnavein yhnavein commented Nov 5, 2014

I've been using angular-tour in a big Angular application and I had to make necessary adjustments, extensions and improvements to the original code to make integration successful. There are no breaking changes, and after my changes apps using original angular-tour should work as before. With those changes angular-tourtip works like a charm in a ~40 controller app. Even having simultaneous tours is possible without any hustle.

Changes:

  • added virtual steps, which allow to specify target element (this allow to have all steps in one place, not messing with having steps spread all over the different views). Moreover, I saw, that sometimes adding tourtip attributes to buttons and links can have influence on their behaviour. This is an easy way to fix that kind of problems
  • added additional callback on the tour level: tourComplete - which run only when user fully completed tour
  • added additional callbacks on the tourtip level: on-show, on-proceed
  • tourtips are now appended to the body, not to the target's element container. It's important, because this way tourtip is more consistent and it's styling doesn't depend on the element it was attached to.
  • added optional backdrop option
  • new available positions: center and center-top - when using them tourtip will be placed visually in the center of target element. It's good for a welcome tourtip on the page.
  • added tests around new features and extended README with examples and explanations

Cheers,

it's possible when placement="center"
fixed way tour-step is added into document - instead adding tourstep
after element, it's added directly into body. This way we won't have an
issue with parent styling affecting our step popover
fixed way position is calculated
default step should be -1 instead of previous 0
and it can be really random one
added way to debug JS unit tests
> npm run-script test-debug
on-show and on-proceed
methods, test structure improved
some refactoring of the *show* function

improvement for virtual steps is additional option, which help determine
target scope which will be used to evaluate onShow or onProgress
callbacks
it's affecting only virtual steps, as in normal case everytime it's the
same scope
this improvement can be disabled for whole application (by new option in tourConfig)
or individually by use-source-scope="true" attribute on selected virtual
tour steps
added throwing exception when target element does not exist
@litch
Copy link

litch commented Nov 30, 2014

This is really nice. I did notice, however, that there is one behavior change - it is now necessary to set the step of the tour to initiate it (say in the Controller). Whereas with the previous version, they would automatically start.

@yhnavein
Copy link
Contributor Author

yhnavein commented Dec 5, 2014

Yeah, you are right. I had to change it as in our application tour is not starting by default (which in my opinion is better - before showing some popups we are asking user if he would like to see the tour). It's much better to set up active step number by hand than hiding it just after starting tour.
Probably I will need to update README with this annotation.

@litch
Copy link

litch commented Dec 5, 2014

I agree - and my implementation is actually doing a check with the backend service to see whether the user is showing the tour, so it’s conditional as well. But the default behavior of the parent project was to show by default, so I was confused when using this fork.

On Dec 5, 2014, at 10:01 AM, Piotrek Dąbrowski notifications@github.com wrote:

Yeah, you are right. I had to change it as in our application tour is not starting by default (which in my opinion is better - before showing some popups we are asking user if he would like to see the tour). It's much better to set up active step number by hand than hiding it just after starting tour.
Probably I will need to update README with this annotation.


Reply to this email directly or view it on GitHub #28 (comment).

@fastf0rward
Copy link
Contributor

+1 for merging this.

fixed code snippets in README
updated template code snippet to the improved version
@yhnavein yhnavein mentioned this pull request Dec 13, 2014
@ikb42
Copy link

ikb42 commented Dec 13, 2014

Great stuff, +1 for merging

@AnnaGerber
Copy link

+1 for merging, this would solve the problems I've been having integrating this module into a complex app

@DaftMonk
Copy link
Owner

Looks very promising. I'll review this and see about merging it after the holidays

@bubbleheadinc
Copy link

Good stuff. Any update on merging? Thanks.

@adamburvill
Copy link

Please merge this!

@kavarell
Copy link

Any word on when this will be merged?

default values were not read by the component
bumped version
@booleanbetrayal
Copy link
Contributor

@DaftMonk - did this PR get lost in the shuffle?

@booleanbetrayal
Copy link
Contributor

@DaftMonk - added support for being able to parent tourtips against a something other than body. would love to see this PR merged!

@barillax
Copy link

Looks great! +1

@mathemagica
Copy link

+1

1 similar comment
@theblang
Copy link

theblang commented May 1, 2015

+1

booleanbetrayal and others added 4 commits May 5, 2015 16:53
fix(scrolling): account for container scroll offsets when updating position of tips + improved tip scrolling
fix(scrolling): fix miscalculation when determining scroll offsets with custom container
@booleanbetrayal
Copy link
Contributor

Merging this!

booleanbetrayal added a commit that referenced this pull request Jul 22, 2015
@booleanbetrayal booleanbetrayal merged commit 100296d into DaftMonk:master Jul 22, 2015
@fastf0rward
Copy link
Contributor

Nice!

@yhnavein
Copy link
Contributor Author

Yay! :)

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

Successfully merging this pull request may close these issues.