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

Added support for changing button texts #8

Merged

Conversation

vneri
Copy link
Contributor

@vneri vneri commented Jun 21, 2019

Hi, thank you for this fork!
I've added an option to the tour, so that the texts of the buttons can be modified (e.g. for localization).
You can now use the following in the Tour parameter (single textes can also be omitted):

buttonTexts{
  nextButton:"Go on",
  prevButton:"Go back",
  pauseButton:"Wait a sec",
  resumeButton:"Continue",
  endTourButton:"It's over",
}

Let me know what you think... Thanks

New option available for the tour, to change the text used for the buttons. This is useful if you want to localize the tours into different languages.
Added support for changing the texts of the buttons
@IGreatlyDislikeJavascript
Copy link
Owner

IGreatlyDislikeJavascript commented Jun 23, 2019

Thanks for your contribution, I like it. It also raises a new question about how we do localization for the entire plugin.

I have just one suggestion - should we change the options object to inside .localization, or similar, rather than just buttonTexts? That will be a little more future proofed and allow us to add more localization later which is neatly inside a single object, rather than expanding the options object, if other features with default text are added to Tourist. For example:

prevButton: this._options.localization.buttonTexts.prevButton||"Prev",

Then in future:

somethingElse: this._options.localization.divTexts.someMessageDiv||"Some future feature",

That way we could add simple code in future to load a localization .js, which simply copies an object into .options, instead of needing to double merge the .options (there's other $.merges happening elsewhere in the code).

Also, just a minor english/consistency point, objTemplatesButtonTextes should be objTemplatesButtonTexts

Finally we'll need to add a note to the docs that this localization option will of course be overridden if a custom template is used.

Please let me know your thoughts. Thank you!

The new option for chancing the button changes has been added to the documentation.
@vneri
Copy link
Contributor Author

vneri commented Jun 24, 2019

Hi great feedback, just added the changes and updated the documentation.
Let me know what you think...
Thanks!

@vneri
Copy link
Contributor Author

vneri commented Jun 24, 2019

Just saw that there is a label option?
https://gist.github.com/emayk/5384683

@IGreatlyDislikeJavascript
Copy link
Owner

Thanks for your updates, those changes look good and I'm very happy to merge them. I'll also duplicate your readme changes into the docs comments at the top of the .js as well, so there's consistency between readme and .js comments (although perhaps at some point soon it might become more sensible to only maintain the readme, as the comment at the top of the js is becoming huge!).

Thanks for your contribution!

Just saw that there is a label option?

Looking at the date of that gist (6 years ago), I'm guessing that some previous version of Bootstrap Tour did have a label option. However I guarantee that most recent/current version of Bootstrap Tour - from which Tourist is forked - definitely doesn't have a label option! It must have been removed by sorich87, I don't know when though. It's good that you've added it back :-)

So don't worry, your work is definitely a valuable addition!

@IGreatlyDislikeJavascript IGreatlyDislikeJavascript merged commit 5adb677 into IGreatlyDislikeJavascript:master Jun 26, 2019
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.

2 participants