Prefixes fixes

[genadis]

@rwwagner90 This pull request contains some fixes related to prefixes.
There are a few more things to fix, which I'll do in separate pull requests (or maybe you will beat me to it):

• modal classes are not prefixed.
• fix removing 'shepherd-modal-target' class from target when moving on to next (at the moment it remains on all previous steps). Maybe the issue here

[RobbieTheWagner]

modal classes are not prefixed.

I wasn't sure if we needed prefixes for the modal. Would you have two tours running at the same time?

[RobbieTheWagner]

This is the demo code, we shouldn't need to do anything here.

[genadis]

we have styling for this class as part of the library here: https://github.com/shipshapecode/shepherd/blob/master/src/js/styles/generateStyles.js#L46

So it's not a regular custom class like other 'class 1' class 2', etc
So it should be prefixed as well.

[RobbieTheWagner]

This class is not generated by shepherd, but passed in as a custom class, so I don't think it should get prefixes, unless we want to support secondary buttons, which maybe we should?

[genadis]

But the style is still part of the library, so I think it should be prefixes like all the others.

I do believe it would be more intuitive, instead of having a "hidden" class for secondary button, to have a more expressive way of setting a button as secondary via configuration.

[RobbieTheWagner]

I definitely agree. We should expose an option to make a button secondary.

[RobbieTheWagner]

@genadis have you tried the code without these changes and hit issues? I think we have all the prefixes we need, more or less, due to nesting of things etc.

[genadis]

We have to have prefixes to all the classes, so if adding this support is not an option we would have to maintain a separate fork :(

The reasoning is:

1. it's confusing to have some of the classes prefixed and some not, when doing custom styling via css.

2. Prevent styling collisions:

Consider the following:
App A - uses the standard build of shepherd.
App B (ours) - loaded on page of App B with prefix: b-shepherd.
The regular styles would be fine. But what if App A owner wants to style shepherd and does something like:

.app-a .shepherd-target {
   background-color: red;
}

App A owner is not bound to any styling constraints. He can just add plain css to his site, we have no control over it (in our cases it's third party application/page).

On the other hand if we prefix everything the above will not effect App B custom built shepherd version which will have b-shepherd-target class.

3. We need to have option to add prefix to tippy as well, since we use a custom tippy build with prefixes for same reasons (preventing styling collisions).

4. normalize prefix to be consistent - is for fixing cases of data attributes with double "-" to be consistent (check the data attribute on the demo page on body) or if by mistake users use "foo" instead of "foo-".

5. Modal prefixing is not to cope with 2 open at the same time but to handle styling collisions of custom css added by third party apps (like in the example above). Although theoretically this could happen (2 open at the same time) since those are 2 separate apps. bad user experience and should be prevented of-course, but could happen.

This doesn't add any complexity to the library, and keeps consistency, so it would be great if this could be merged.
Thanks

[genadis]

The styling collision I've described in my previous comment are based on the extensive experience we had integrating our product on various systems. So it really is a necessity for us to prevent future issues before they occur.

[RobbieTheWagner]

This overall looks good, don't worry, we'll get all the prefixes in 😃

@genadis would you perhaps be open to helping write some tests for adding prefixes to ensure all the things are properly prefixed when you expect them to be? This will help us ensure this functionality does not break in the future.

[RobbieTheWagner]

I believe we could simplify this method to:

if(!isString(prefix) || prefix === '') {
  return '';
}

return prefix.endsWith('-') ? prefix : `${prefix}-`;

[genadis]

I've simplified.
but endsWith is not supported by IE11 using additional polifill just for this line seems too much.

[RobbieTheWagner]

While maintaining a separate classPrefix and tippyPrefix is technically okay, I'm curious, do you have a need for them to be different or could they both be demo-? etc

[genadis]

I guess practically they would be the same, but we still need option to choose if to prefix tippy or not (separate from the shepherd prefix).

specifically for our use case, it will always be the same, but some users of this project might have different more loose requirement. we don't want to enforce them using custom tippy build if they want to just prefix the shepherd.

[genadis]

This overall looks good, don't worry, we'll get all the prefixes in 😃

@genadis would you perhaps be open to helping write some tests for adding prefixes to ensure all the things are properly prefixed when you expect them to be? This will help us ensure this functionality does not break in the future.

great news!
I'll do my best adding tests, this might take some time though.

[RobbieTheWagner]

@genadis I am going to go ahead and merge this, then I am going to work on exposing the secondary button stuff. I will also try to write some tests for the prefixes.