-
-
Notifications
You must be signed in to change notification settings - Fork 649
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
Prefixes fixes #453
Prefixes fixes #453
Changes from 6 commits
090ee80
11462b8
670fbb6
5323025
8e83c35
ed9aaf8
a4a3159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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: { | ||
|
@@ -43,7 +45,7 @@ export function generateStyles(options) { | |
'&:hover': { | ||
background: darken(0.1, variables.shepherdThemePrimary) | ||
}, | ||
'&.shepherd-button-secondary': { | ||
[`&.${classPrefix}shepherd-button-secondary`]: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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': { | ||
|
@@ -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' | ||
} | ||
|
@@ -164,20 +166,20 @@ export function generateStyles(options) { | |
'&[x-placement^="top"]': { | ||
marginBottom: arrowMargin, | ||
|
||
'.tippy-arrow': { | ||
[`.${tippyPrefix}tippy-arrow`]: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While maintaining a separate There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} | ||
|
@@ -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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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) { | ||
|
@@ -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( | ||
|
@@ -230,3 +232,13 @@ function _makeCenteredTippy(step) { | |
|
||
return tippy(document.body, tippyOptions); | ||
} | ||
|
||
export function normalizePrefix(prefix) { | ||
if (typeof prefix !== 'string' || prefix === '') { | ||
return ''; | ||
} | ||
if (prefix.charAt(prefix.length - 1) !== '-') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we could simplify this method to: if(!isString(prefix) || prefix === '') {
return '';
}
return prefix.endsWith('-') ? prefix : `${prefix}-`; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've simplified. |
||
return `${prefix}-`; | ||
} | ||
return prefix; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.