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.

Sign up for GitHub

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

Jump to bottom

Prefixes fixes #453

Merged
merged 7 commits into from
Jul 22, 2019
Merged

Prefixes fixes #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
090ee80
normalize prefix to be consistent
genadis Jul 21, 2019
11462b8
cleanup removing unused data-button-event attribute (never added)
genadis Jul 21, 2019
670fbb6
fix removing tour classes from the body when done
genadis Jul 21, 2019
5323025
fix class prefixes for has-title, has-cancel-ilnk,enabled,target and …
genadis Jul 21, 2019
8e83c35
Merge branch 'master' into prefixes-fixes
genadis Jul 21, 2019
ed9aaf8
added option to prefix classes for tippy styling - tippyClassPrefix
genadis Jul 21, 2019
a4a3159
simplify normilizePrefix
genadis Jul 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions demo/js/welcome.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var shepherd = new Shepherd.Tour({
defaultStepOptions: {
classes: 'class-1 class-2',
Expand All @@ -21,7 +23,7 @@
maxWidth: 500
}
},
classPrefix: 'demo-',
classPrefix: prefix,
styleVariables: {
shepherdThemePrimary: '#00213b',
shepherdThemeSecondary: '#e5e5e5'
Expand All @@ -37,7 +39,7 @@
classes: 'shepherd shepherd-welcome',
buttons: [{
action: shepherd.cancel,
classes: 'shepherd-button-secondary',
classes: buttonSecondaryClass,
text: 'Exit'
}, {
action: shepherd.next,
Expand All @@ -53,7 +55,7 @@
},
buttons: [{
action: shepherd.back,
classes: 'shepherd-button-secondary',
classes: buttonSecondaryClass,
text: 'Back'
}, {
action: shepherd.next,
Expand All @@ -69,7 +71,7 @@
},
buttons: [{
action: shepherd.back,
classes: 'shepherd-button-secondary',
classes: buttonSecondaryClass,
text: 'Back'
}, {
action: shepherd.next,
Expand All @@ -85,7 +87,7 @@
},
buttons: [{
action: shepherd.back,
classes: 'shepherd-button-secondary',
classes: buttonSecondaryClass,
text: 'Back'
}, {
action: shepherd.next,
Expand All @@ -97,7 +99,7 @@
text: 'But attachment is totally optional!\n Without a target, a tour step will create an element that\'s centered within the view. Check out the <a href="https://shepherdjs.dev/docs/">documentation</a> to learn more.',
buttons: [{
action: shepherd.back,
classes: 'shepherd-button-secondary',
classes: buttonSecondaryClass,
text: 'Back'
}, {
action: shepherd.next,
Expand All @@ -113,7 +115,7 @@
},
buttons: [{
action: shepherd.back,
classes: 'shepherd-button-secondary',
classes: buttonSecondaryClass,
text: 'Back'
}, {
action: shepherd.next,
Expand Down
16 changes: 8 additions & 8 deletions src/js/step.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import autoBind from './utils/auto-bind';
import { isElement, isFunction, isUndefined } from './utils/type-check';
import { bindAdvance, bindButtonEvents, bindCancelLink } from './utils/bind.js';
import { getElementForStep } from './utils/dom';
import { createFromHTML, setupTooltip, parseAttachTo } from './utils/general.js';
import { createFromHTML, setupTooltip, parseAttachTo, normalizePrefix } from './utils/general.js';
import { toggleShepherdModalClass } from './utils/modal';

// Polyfills
Expand Down Expand Up @@ -109,7 +109,7 @@ export class Step extends Evented {
constructor(tour, options = {}) {
super(tour, options);
this.tour = tour;
this.classPrefix = this.tour.options && this.tour.options.classPrefix ? `${this.tour.options.classPrefix}-` : '';
this.classPrefix = this.tour.options ? normalizePrefix(this.tour.options.classPrefix) : '';
this.styles = tour.styles;

autoBind(this);
Expand Down Expand Up @@ -249,7 +249,7 @@ export class Step extends Evented {
const link = createFromHTML(`<a href class="${this.styles['cancel-link'].trim()}"></a>`);
header.appendChild(link);

element.classList.add('shepherd-has-cancel-link');
element.classList.add(`${this.classPrefix}shepherd-has-cancel-link`);
bindCancelLink(link, this);
}
}
Expand Down Expand Up @@ -454,9 +454,9 @@ export class Step extends Evented {
this._setupElements();
}

this.target.classList.add('shepherd-enabled', 'shepherd-target');

document.body.setAttribute(`data-${this.classPrefix}shepherd-step`, this.id);
const prefix = this.classPrefix;
this.target.classList.add(`${prefix}shepherd-enabled`, `${prefix}shepherd-target`);
document.body.setAttribute(`data-${prefix}shepherd-step`, this.id);

if (this.options.scrollTo) {
setTimeout(() => {
Expand Down Expand Up @@ -503,7 +503,7 @@ export class Step extends Evented {
if (this.options.highlightClass) {
this.target.classList.remove(this.options.highlightClass);
}

this.target.classList.remove('shepherd-enabled', 'shepherd-target');
const prefix = this.classPrefix;
this.target.classList.remove(`${prefix}shepherd-enabled`, `${prefix}shepherd-target`);
}
}
28 changes: 15 additions & 13 deletions src/js/styles/generateStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { darken, lighten } from 'polished';
import getVariables from './variables';
import { setupNano } from './nano';
import { normalizePrefix } from '../utils/general';

export function generateStyles(options) {
const variables = getVariables(options);
const nano = setupNano(options.classPrefix);
const classPrefix = options.classPrefix || '';
const classPrefix = normalizePrefix(options.classPrefix);
const tippyPrefix = normalizePrefix(options.tippyClassPrefix);

const styles = {
active: {
Expand Down Expand Up @@ -43,7 +45,7 @@ export function generateStyles(options) {
'&:hover': {
background: darken(0.1, variables.shepherdThemePrimary)
},
'&.shepherd-button-secondary': {
[`&.${classPrefix}shepherd-button-secondary`]: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

background: variables.shepherdThemeSecondary,
color: variables.shepherdThemeTextSecondary,
'&:hover': {
Expand Down Expand Up @@ -109,7 +111,7 @@ export function generateStyles(options) {
justifyContent: 'flex-end',
lineHeight: '2em',
padding: '0.75em 0.75em 0',
[`.shepherd-has-title .${classPrefix}shepherd-content &`]: {
[`.${classPrefix}shepherd-has-title .${classPrefix}shepherd-content &`]: {
background: variables.shepherdHeaderBackground,
padding: '1em'
}
Expand Down Expand Up @@ -164,20 +166,20 @@ export function generateStyles(options) {
'&[x-placement^="top"]': {
marginBottom: arrowMargin,

'.tippy-arrow': {
[`.${tippyPrefix}tippy-arrow`]: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@genadis genadis Jul 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

borderTopColor: variables.shepherdTextBackground
}
},

'&[x-placement^="bottom"]': {
marginTop: arrowMargin,

'.tippy-arrow': {
[`.${tippyPrefix}tippy-arrow`]: {
borderBottomColor: variables.shepherdTextBackground
},

'&.shepherd-has-title': {
'.tippy-arrow': {
[`&.${classPrefix}shepherd-has-title`]: {
[`.${tippyPrefix}tippy-arrow`]: {
borderBottomColor: variables.shepherdHeaderBackground
}
}
Expand All @@ -186,35 +188,35 @@ export function generateStyles(options) {
'&[x-placement^="left"]': {
marginRight: arrowMargin,

'.tippy-arrow': {
[`.${tippyPrefix}tippy-arrow`]: {
borderLeftColor: variables.shepherdTextBackground
}
},

'&[x-placement^="right"]': {
marginLeft: arrowMargin,

'.tippy-arrow': {
[`.${tippyPrefix}tippy-arrow`]: {
borderRightColor: variables.shepherdTextBackground
}
}
};

// We have to add the root shepherd class separately
classes.shepherd = nano.rule({
'&.tippy-popper': {
[`&.${tippyPrefix}tippy-popper`]: {
...popperThemeArrows,
zIndex: variables.shepherdElementZIndex,

'.tippy-tooltip': {
[`.${tippyPrefix}tippy-tooltip`]: {
backgroundColor: variables.shepherdTextBackground,

'.tippy-arrow': {
[`.${tippyPrefix}tippy-arrow`]: {
transform: `scale(${variables.arrowSize})`,
zIndex: variables.shepherdElementZIndex + 1
},

'.tippy-content': {
[`.${tippyPrefix}tippy-content`]: {
maxHeight: variables.shepherdElementMaxHeight,
maxWidth: variables.shepherdElementMaxWidth,
padding: 0,
Expand Down
3 changes: 2 additions & 1 deletion src/js/styles/nano.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import { addon as nesting } from 'nano-css/addon/nesting';
import { addon as prefixer } from 'nano-css/addon/prefixer';
import { addon as rule } from 'nano-css/addon/rule';
import { addon as sheet } from 'nano-css/addon/sheet';
import { normalizePrefix } from '../utils/general';

export function setupNano(classPrefix) {
const nano = create({
// Add prefix to all generated class names.
pfx: classPrefix || ''
pfx: normalizePrefix(classPrefix)
});

cache(nano);
Expand Down
5 changes: 3 additions & 2 deletions src/js/tour.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import autoBind from './utils/auto-bind';
import { isFunction, isNumber, isString, isUndefined } from './utils/type-check';
import { defaults as tooltipDefaults } from './utils/tooltip-defaults';
import { cleanupSteps, cleanupStepEventListeners } from './utils/cleanup';
import { normalizePrefix } from './utils/general';
import { generateStyles } from './styles/generateStyles';

/**
Expand Down Expand Up @@ -50,7 +51,7 @@ export class Tour extends Evented {
autoBind(this);

this.options = options;
this.classPrefix = this.options && this.options.classPrefix ? `${this.options.classPrefix}-` : '';
this.classPrefix = this.options ? normalizePrefix(this.options.classPrefix) : '';
this.styles = generateStyles(options);
this.steps = this.options.steps || [];

Expand Down Expand Up @@ -376,7 +377,7 @@ export class Tour extends Evented {
*/
_removeBodyAttrs() {
document.body.removeAttribute(`data-${this.classPrefix}shepherd-active-tour`);
document.body.classList.remove('shepherd-active');
document.body.classList.remove(this.styles.active.trim());
}

}
Expand Down
1 change: 0 additions & 1 deletion src/js/utils/bind.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ export function bindButtonEvents(cfg, el, step) {

// Cleanup event listeners on destroy
step.on('destroy', () => {
el.removeAttribute('data-button-event');
el.removeEventListener(event, handler);
});
});
Expand Down
15 changes: 12 additions & 3 deletions src/js/utils/general.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { isString, isUndefined } from './type-check';
import tippy from 'tippy.js';
import { missingTippy } from './error-messages';

const addHasTitleClass = _createClassModifier('shepherd-has-title');
const addHasTitleClass = (step) => {
return { addHasTitleClass: _createClassModifier(`${step.classPrefix}shepherd-has-title`) };
};

function _getCenteredStylePopperModifier(styles) {
return {
Expand Down Expand Up @@ -177,7 +179,7 @@ function _makeAttachedTippyOptions(attachToOptions, step) {
Object.assign(resultingTippyOptions, step.options.tippyOptions);

if (step.options.title) {
Object.assign(defaultPopperOptions.modifiers, { addHasTitleClass });
Object.assign(defaultPopperOptions.modifiers, addHasTitleClass(step));
}

if (step.options.tippyOptions && step.options.tippyOptions.popperOptions) {
Expand Down Expand Up @@ -211,7 +213,7 @@ function _makeCenteredTippy(step) {
tippyOptions.popperOptions = tippyOptions.popperOptions || {};

if (step.options.title) {
Object.assign(defaultPopperOptions.modifiers, { addHasTitleClass });
Object.assign(defaultPopperOptions.modifiers, addHasTitleClass(step));
}

Object.assign(
Expand All @@ -230,3 +232,10 @@ function _makeCenteredTippy(step) {

return tippy(document.body, tippyOptions);
}

export function normalizePrefix(prefix) {
if (!isString(prefix) || prefix === '') {
return '';
}
return prefix.charAt(prefix.length - 1) !== '-' ? `${prefix}-` : prefix;
}