sorich87 / bootstrap-tour

Quick and easy product tours with Twitter Bootstrap Popovers
http://bootstraptour.com
MIT License
4.44k stars 942 forks source link

Pull request: many features and fixes added #713

Open IGreatlyDislikeJavascript opened 5 years ago

IGreatlyDislikeJavascript commented 5 years ago

I've fixed a few issues reported in this repository, added a bunch of new features requested and that I needed, and fixed a few things that weren't reported yet.

Full comments and explanation in the comments at the top of the js. Note: this is only the bootstrap version that I'm updating.

https://github.com/IGreatlyDislikeJavascript/bootstrap-tour/blob/master/build/js/bootstrap-tour.js

Full list of all features added below. Review comments in top of bootstrap-tour.js for full explanation and examples:

  1. onNext/onPrevious - prevent auto-move to next step, allow .goTo
  2. Do not call Tour.init - fixed tours with hidden elements on page reload
  3. Dynamically determine step element by function
  4. Only continue tour when reflex element is clicked using reflexOnly
  5. Call onElementUnavailable if step element is missing
  6. Scroll flicker/continual step reload fixed
  7. Magic progress bar and progress text, plus options to customize per step
  8. Prevent user interaction with element using preventInteraction
  9. Wait for arbitrary DOM element to be visible before showing tour step/crapping out due to missing element, using delayOnElement
  10. Handle bootstrap modal dialogs better - autodetect modals or children of modals, and call onModalHidden when user dismisses modal without following tour steps
  11. Automagically fixes drawing issues with Bootstrap Selectpicker (https://github.com/snapappointments/bootstrap-select/)
IGreatlyDislikeJavascript commented 5 years ago

Final updates added. I won't be adding any more, I've just discovered this nutso thing called CoffeeScript that this original plugin uses (because the one thing we all need is more complex abstraction of javascript).

Good luck!

Please remember to download the CSS as well, includes a class for preventInteraction.

Changes from 0.5:

sorich87 commented 5 years ago

Thanks for your contribution @IGreatlyDislikeJavascript . Using CoffeeScript made sense when the plugin was just released. I'm looking to move it to ES6 but don't have time to do that right now. If you want to contribute that, it'll be very welcome.

IGreatlyDislikeJavascript commented 5 years ago

Hi @sorich87 . My pleasure, glad these updates are of use.

There's a few more things I'd update, especially around the code flow of how step show/hide triggers flow, but that's quite a lot of work to refactor and as of now all my changes meet the reqs for what I'm working on.

Your plugin is great, saved me a lot of work - please don't misunderstand my comments about coffee script, they're absolutely not a criticism of your approach. I'd just never heard of coffee script until yesterday, and I don't really get its value.

When I realized what coffee script was, it became apparent that, to merge my js code with yours, either all my code would have to be rewritten in coffee script or your plugin would have to move to js. So inadvertently I've forked your plugin in a way that doesn't let us work collaboratively, which is a shame and the opposite of what I was trying to do!

Genuinely I have no real skill or knowledge of JavaScript. I've coded in C++ / x86 asm for years, it's only that experience that allowed me to figure out how your plugin code works, and to make changes. (It doesn't help I don't really know how to use github repos proudly either, yes I'm just an old coder trying to blunder through impending obsolescence!)

So when you say "move to ES6", I think you mean "stop using coffee script and use JavaScript directly", right?

If that's correct, again I'm a bit confused - isn't the version I've forked and updated exactly that?

I can see how coffee script has interpreted itself into some strange approaches, and refactored things in odd ways. But fundamentally if we now just used my fork as the basis, wouldn't that mean it's "moved to ES6"? Your plugin already uses promises etc which I understand is an ES6 thing, so isn't it ES6 already?

Sorry, seriously, I have no great understanding of all this, and I do greatly dislike JavaScript!

Thanks again for a great plugin, and hopefully our joint efforts are of use to someone.

azizmashkour commented 5 years ago

@IGreatlyDislikeJavascript , thank you again for all your efforts despite you're not a Javascript or CoffeeScript developer. In fact BootstrapTour is written in CoffeeScript but the plugin uses a transpiler to parse all the code intoES6, so it's not a pure ES6 base code, that's why @sorich87 suggested definitively rewriting all the code into ES6 so it will facilitate the merging and anyone could easily contribute to any improvement.

IGreatlyDislikeJavascript commented 5 years ago

@azizmashkour thanks, I think I understand now - what you're both talking about is a near rewrite from the ground up in native JavaScript, which will also have the benefit of removing some of the odd code structure resulting from the transpilation from coffee script.

Sounds like a great idea, but I think that rewriting everything (or at least refactoring and code tidying the js) is beyond my knowledge. If you do decide to do it though I'd be happy to help, and walk through the code level changes I made to save you digging through my less than optimal js! There's quite a few core changes that aren't obviously exposed, e.g: I reversed the $.extend on steps to allow retaining important info.

Perhaps in the meantime an option is to include a link in your readme to the .js and .css inn my fork, so that people have the choice of sticking with your official repo as first and best option, or easily finding my fork if it fixes a specific prob they have.

Thanks again for a great plugin.

IGreatlyDislikeJavascript commented 5 years ago

I've created a new repo for the fork of bootstrap-tour:

https://github.com/IGreatlyDislikeJavascript/bootstrap-tourist

This should help avoid confusion in the fork etc.