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

Remove object-assign-deep, refactor setting popper options #516

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ module.exports = {
'complexity': ['warn', 6],
'max-lines': ['warn', { max: 250, skipBlankLines: true, skipComments: true }],
'no-console': 'off',
'prefer-const': 'off',
'react/jsx-tag-spacing': 'off'
},
overrides: [
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
"es6-object-assign": "^1.1.0",
"es6-symbol": "^3.1.1",
"nano-css": "^5.2.0",
"object-assign-deep": "^0.4.0",
"polished": "^3.4.1",
"preact": "^8.5.1",
"smoothscroll-polyfill": "^0.4.4",
Expand Down
4 changes: 2 additions & 2 deletions rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const plugins = [
commonjs(),
eslint(),
babel({
exclude: /node_modules\/(?!(nano-css|object-assign-deep)\/).*/
exclude: /node_modules\/(?!(nano-css)\/).*/
}),
replace({
'process.env.NODE_ENV': JSON.stringify(env)
Expand Down Expand Up @@ -102,7 +102,7 @@ if (!process.env.DEVELOPMENT) {
}),
commonjs(),
babel({
exclude: /node_modules\/(?!(nano-css|object-assign-deep)\/).*/
exclude: /node_modules\/(?!(nano-css)\/).*/
}),
replace({
'process.env.NODE_ENV': JSON.stringify(env)
Expand Down
137 changes: 9 additions & 128 deletions src/js/utils/general.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,7 @@
import { makeAttachedTippyOptions, makeCenteredTippy } from './tippy-popper-options';
import { isString, isUndefined } from './type-check';
import objectAssignDeep from 'object-assign-deep';
import tippy from 'tippy.js';

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

function _getCenteredStylePopperModifier(styles) {
return {
computeStyle: {
enabled: true,
fn(data) {
data.styles = Object.assign({}, data.styles, {
left: '50%',
top: '50%',
transform: 'translate(-50%, -50%)'
});

return data;
}
},
addShepherdClass: _createClassModifier(styles.shepherd.trim())
};
}

/**
* Used to compose settings for tippyOptions.popperOptions (https://atomiks.github.io/tippyjs/#popper-options-option)
* @private
*/
function _getDefaultPopperOptions(styles) {
return {
positionFixed: true,
modifiers: {
addShepherdClass: _createClassModifier(styles.shepherd.trim())
}
};
}

/**
* Ensure class prefix ends in `-`
* @param {string} prefix The prefix to prepend to the class names generated by nano-css
Expand Down Expand Up @@ -98,22 +63,6 @@ export function setupTooltip(step) {
step.target = attachToOpts.element;
}

/**
* Create a popper modifier for adding the passed className to the popper
* @param {string} className The class to add to the popper
* @return {{fn(*): *, enabled: boolean}|*}
* @private
*/
function _createClassModifier(className) {
return {
enabled: true,
fn(data) {
data.instance.popper.classList.add(className);
return data;
}
};
}

/**
* Generates a `Tippy` instance from a set of base `attachTo` options
* @param attachToOptions
Expand All @@ -122,84 +71,16 @@ function _createClassModifier(className) {
* @private
*/
function _makeTippyInstance(attachToOptions, step) {
if (!attachToOptions.element || !attachToOptions.on) {
return _makeCenteredTippy(step);
}

const tippyOptions = _makeAttachedTippyOptions(attachToOptions, step);

return tippy(attachToOptions.element, tippyOptions);
}
let tippyOptions = {};
let element = document.body;

/**
* Generates the hash of options that will be passed to `Tippy` instances
* target an element in the DOM.
*
* @param {Object} attachToOptions The local `attachTo` options
* @param {Step} step The step instance
* @return {Object} The final tippy options object
* @private
*/
function _makeAttachedTippyOptions(attachToOptions, step) {
const defaultPopperOptions = _getDefaultPopperOptions(step.styles);
const tippyOptions = {
content: step.el,
flipOnUpdate: true,
placement: attachToOptions.on || 'right',
...step.options.tippyOptions
};

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

if (step.options.tippyOptions && step.options.tippyOptions.popperOptions) {
objectAssignDeep(defaultPopperOptions, step.options.tippyOptions.popperOptions);
if (!attachToOptions.element || !attachToOptions.on) {
tippyOptions = makeCenteredTippy(step);
} else {
tippyOptions = makeAttachedTippyOptions(attachToOptions, step);
element = attachToOptions.element;
}

tippyOptions.popperOptions = defaultPopperOptions;

return tippyOptions;
return tippy(element, tippyOptions);
}

/**
* Generates a `Tippy` instance for a tooltip that doesn't have a
* target element in the DOM -- and thus is positioned in the center
* of the view
*
* @param {Step} step The step instance
* @return {tippy} The final tippy instance
* @private
*/
function _makeCenteredTippy(step) {
const centeredStylePopperModifier = _getCenteredStylePopperModifier(step.styles);
const defaultPopperOptions = _getDefaultPopperOptions(step.styles);
const tippyOptions = {
content: step.el,
placement: 'top',
...step.options.tippyOptions
};

tippyOptions.arrow = false;
tippyOptions.popperOptions = tippyOptions.popperOptions || {};

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

objectAssignDeep(
defaultPopperOptions.modifiers,
centeredStylePopperModifier,
tippyOptions.popperOptions.modifiers
);

const finalPopperOptions = objectAssignDeep(
{},
defaultPopperOptions,
tippyOptions.popperOptions
);

tippyOptions.popperOptions = finalPopperOptions;

return tippy(document.body, tippyOptions);
}
126 changes: 126 additions & 0 deletions src/js/utils/tippy-popper-options.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
const addHasTitleClass = (step) => {
return { addHasTitleClass: _createClassModifier(`${step.classPrefix}shepherd-has-title`) };
};

/**
* Create a popper modifier for adding the passed className to the popper
* @param {string} className The class to add to the popper
* @return {{fn(*): *, enabled: boolean}|*}
* @private
*/
function _createClassModifier(className) {
return {
enabled: true,
fn(data) {
data.instance.popper.classList.add(className);
return data;
}
};
}

function _getCenteredStylePopperModifier(styles) {
return {
computeStyle: {
enabled: true,
fn(data) {
data.styles = Object.assign({}, data.styles, {
left: '50%',
top: '50%',
transform: 'translate(-50%, -50%)'
});

return data;
}
},
addShepherdClass: _createClassModifier(styles.shepherd.trim())
};
}

/**
* Used to compose settings for tippyOptions.popperOptions (https://atomiks.github.io/tippyjs/#popper-options-option)
* @private
*/
function _getDefaultPopperOptions(styles) {
return {
positionFixed: true,
modifiers: {
addShepherdClass: _createClassModifier(styles.shepherd.trim())
}
};
}

/**
* Generates the hash of options that will be passed to `Tippy` instances
* target an element in the DOM.
*
* @param {Object} attachToOptions The local `attachTo` options
* @param {Step} step The step instance
* @return {Object} The final tippy options object
*/
export function makeAttachedTippyOptions(attachToOptions, step) {
let { popperOptions, tippyOptions } = _makeCommonTippyOptions(step);

tippyOptions.flipOnUpdate = true;
tippyOptions.placement = attachToOptions.on || 'right';

if (step.options.tippyOptions && step.options.tippyOptions.popperOptions) {
popperOptions = {
...popperOptions,
...step.options.tippyOptions.popperOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if step.options.tippyOptions.popperOptions has modifiers they will override the tippy modifiers, which was the issue in the first place.
please look at my original Pull request that solves the issue.

};
}

tippyOptions.popperOptions = popperOptions;

return tippyOptions;
}

/**
* Generates the hash of options for a tooltip that doesn't have a
* target element in the DOM -- and thus is positioned in the center
* of the view
*
* @param {Step} step The step instance
* @return {Object} The final tippy options object
*/
export function makeCenteredTippy(step) {
const centeredStylePopperModifier = _getCenteredStylePopperModifier(step.styles);
let { popperOptions, tippyOptions } = _makeCommonTippyOptions(step);

tippyOptions.placement = 'top';
tippyOptions.arrow = false;
tippyOptions.popperOptions = tippyOptions.popperOptions || {};

popperOptions.modifiers = {
...popperOptions.modifiers,
...centeredStylePopperModifier,
...tippyOptions.popperOptions.modifiers
};

popperOptions = {
...popperOptions,
...tippyOptions.popperOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if tippyOptions.popperOptions has modifiers they will override the tippy modifiers, which was the issue in the first place.
please look at my original Pull request that solves the issue.

};

tippyOptions.popperOptions = popperOptions;

return tippyOptions;
}

function _makeCommonTippyOptions(step) {
const popperOptions = _getDefaultPopperOptions(step.styles);

const tippyOptions = {
content: step.el,
...step.options.tippyOptions
};

if (step.options.title) {
popperOptions.modifiers = {
...popperOptions.modifiers,
...addHasTitleClass(step)
};
}

return { popperOptions, tippyOptions };
}
1 change: 0 additions & 1 deletion test/unit/utils/bind.spec.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { bindAdvance } from '../../../src/js/utils/bind.js';
import { Step } from '../../../src/js/step.jsx';
import { spy } from 'sinon';
import { Tour } from '../../../src/js/tour';

describe('Bind Utils', function() {
describe('bindAdvance()', () => {
Expand Down
34 changes: 34 additions & 0 deletions test/unit/utils/tippy-popper-options.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { makeAttachedTippyOptions } from '../../../src/js/utils/tippy-popper-options';

describe('Tippy/Popper Options Utils', function() {
describe('makeAttachedTippyOptions()', function() {
it('passing tippyOptions.popperOptions sets nested values', function() {
const stepOptions =
{
options: {
tippyOptions: {
popperOptions: {
modifiers: {
foo: 'bar',
preventOverflow: {
escapeWithReference: true
}
}
}
}
},
styles: {
shepherd: ' shepherd'
}
};

const attachToOpts = {
on: 'top'
};

const tippyOptions = makeAttachedTippyOptions(attachToOpts, stepOptions);
expect(tippyOptions.popperOptions.modifiers.foo).toBe('bar');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was that if you have custom modifiers, shepherd poper modifiers get removed.
The tests does not check it

expect(tippyOptions.popperOptions.modifiers.preventOverflow.escapeWithReference).toBe(true);
});
});
});
5 changes: 0 additions & 5 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5617,11 +5617,6 @@ oauth-sign@~0.9.0:
resolved "https://registry.yarnpkg.com/oauth-sign/-/oauth-sign-0.9.0.tgz#47a7b016baa68b5fa0ecf3dee08a85c679ac6455"
integrity sha512-fexhUFFPTGV8ybAtSIGbV6gOkSv8UtRbDBnAyLQw4QPKkgNlsH2ByPGtMUqdWkos6YCRmAqViwgZrJc/mRDzZQ==

object-assign-deep@^0.4.0:
version "0.4.0"
resolved "https://registry.yarnpkg.com/object-assign-deep/-/object-assign-deep-0.4.0.tgz#43505d3679abb9686ab359b97ac14cc837a9d143"
integrity sha512-54Uvn3s+4A/cMWx9tlRez1qtc7pN7pbQ+Yi7mjLjcBpWLlP+XbSHiHbQW6CElDiV4OvuzqnMrBdkgxI1mT8V/Q==

object-assign@^4.0.1, object-assign@^4.1.0, object-assign@^4.1.1:
version "4.1.1"
resolved "https://registry.yarnpkg.com/object-assign/-/object-assign-4.1.1.tgz#2109adc7965887cfc05cbbd442cac8bfbb360863"
Expand Down