Prefixes fixes #453

genadis · 2019-07-21T20:56:38Z

@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

…button-secondary

* master: 4.0.0-beta.11 Transpile nano-css to fix IE11 4.0.0-beta.10

RobbieTheWagner · 2019-07-22T00:02:38Z

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 · 2019-07-22T00:07:39Z

demo/js/welcome.js

@@ -9,6 +9,8 @@
  }

  function setupShepherd() {
+    var prefix = 'demo-';
+    var buttonSecondaryClass = prefix + 'shepherd-button-secondary';


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

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 · 2019-07-22T00:08:46Z

src/js/styles/generateStyles.js

@@ -43,7 +45,7 @@ export function generateStyles(options) {
      '&:hover': {
        background: darken(0.1, variables.shepherdThemePrimary)
      },
-      '&.shepherd-button-secondary': {
+      [`&.${classPrefix}shepherd-button-secondary`]: {


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?

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.

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

RobbieTheWagner · 2019-07-22T00:09:59Z

@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 · 2019-07-22T01:29:42Z

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:

it's confusing to have some of the classes prefixed and some not, when doing custom styling via css.
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.

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).
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-".
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 · 2019-07-22T01:40:57Z

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 · 2019-07-22T02:14:12Z

src/js/utils/general.js

+  if (typeof prefix !== 'string' || prefix === '') {
+    return '';
+  }
+  if (prefix.charAt(prefix.length - 1) !== '-') {


I believe we could simplify this method to:

if(!isString(prefix) || prefix === '') { return ''; } return prefix.endsWith('-') ? prefix : `${prefix}-`;

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

RobbieTheWagner · 2019-07-22T02:16:42Z

src/js/styles/generateStyles.js

@@ -164,20 +166,20 @@ export function generateStyles(options) {
    '&[x-placement^="top"]': {
      marginBottom: arrowMargin,

-      '.tippy-arrow': {
+      [`.${tippyPrefix}tippy-arrow`]: {


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

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 · 2019-07-22T09:06:46Z

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 · 2019-07-22T13:41:10Z

@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.

* master: Cleanup some issues from prefixing Prefixes fixes (shipshapecode#453)

genadis added 6 commits July 21, 2019 11:51

normalize prefix to be consistent

090ee80

cleanup removing unused data-button-event attribute (never added)

11462b8

fix removing tour classes from the body when done

670fbb6

fix class prefixes for has-title, has-cancel-ilnk,enabled,target and …

5323025

…button-secondary

Merge branch 'master' into prefixes-fixes

8e83c35

* master: 4.0.0-beta.11 Transpile nano-css to fix IE11 4.0.0-beta.10

added option to prefix classes for tippy styling - tippyClassPrefix

ed9aaf8

RobbieTheWagner reviewed Jul 22, 2019

View reviewed changes

simplify normilizePrefix

a4a3159

RobbieTheWagner added the enhancement label Jul 22, 2019

RobbieTheWagner mentioned this pull request Jul 22, 2019

fix removing 'shepherd-modal-target' #455

Closed

RobbieTheWagner merged commit 2ea80c0 into shipshapecode:master Jul 22, 2019

genadis added a commit to genadis/shepherd that referenced this pull request Jul 22, 2019

Merge branch 'master' into step-index-in-events

5432e4c

* master: Cleanup some issues from prefixing Prefixes fixes (shipshapecode#453)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefixes fixes #453

Prefixes fixes #453

genadis commented Jul 21, 2019 •

edited

Loading

RobbieTheWagner commented Jul 22, 2019

RobbieTheWagner Jul 22, 2019

genadis Jul 22, 2019

RobbieTheWagner Jul 22, 2019

genadis Jul 22, 2019

RobbieTheWagner Jul 22, 2019

RobbieTheWagner commented Jul 22, 2019

genadis commented Jul 22, 2019

genadis commented Jul 22, 2019

RobbieTheWagner left a comment

RobbieTheWagner Jul 22, 2019

genadis Jul 22, 2019

RobbieTheWagner Jul 22, 2019

genadis Jul 22, 2019 •

edited

Loading

genadis commented Jul 22, 2019

RobbieTheWagner commented Jul 22, 2019

Prefixes fixes #453

Prefixes fixes #453

Conversation

genadis commented Jul 21, 2019 • edited Loading

RobbieTheWagner commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobbieTheWagner commented Jul 22, 2019

genadis commented Jul 22, 2019

genadis commented Jul 22, 2019

RobbieTheWagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genadis Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

genadis commented Jul 22, 2019

RobbieTheWagner commented Jul 22, 2019

genadis commented Jul 21, 2019 •

edited

Loading

genadis Jul 22, 2019 •

edited

Loading