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

Constrain input for Step.options.buttons to an array of buttons. #271

Merged
merged 3 commits into from
Oct 12, 2018
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
14 changes: 0 additions & 14 deletions docs/welcome/css/welcome.css

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions index.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,7 @@ the step will execute. For example:
},
```
- `classes`: Extra classes to add to the step. `shepherd-theme-arrows` will give you our theme.
- `buttons`: An array of buttons to add to the step. By default we add a Next button which triggers `next()`, set this to false
to disable. Each button in the array is an object of the format:
- `buttons`: An array of buttons to add to the step. These will be rendered in a footer below the main body text. Each button in the array is an object of the format:
- `text`: The HTML text of the button
- `classes`: Extra classes to apply to the `<a>`
- `action`: A function executed when the button is clicked on
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"test:cy:watch": "yarn test:build && start-server-and-test start-test-server http://localhost:9002 cy:open",
"test:unit:ci": "cross-env NODE_ENV=test webpack --config webpack.test.config.js && yarn mocha-headless-chrome",
"test:unit:watch": "webpack-dev-server --config webpack.test.config.js",
"view-coverage": "http-server -p 9003 ./coverage/lcov-report -o",
"watch": "yarn clean && webpack --watch --mode development"
},
"homepage": "http://shipshapecode.github.io/shepherd/docs/welcome/",
Expand Down
34 changes: 3 additions & 31 deletions src/js/step.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {
isElement,
isEmpty,
isFunction,
isPlainObject,
isString,
isUndefined
} from 'lodash';
Expand Down Expand Up @@ -48,8 +47,8 @@ export class Step extends Evented {
* You can also always manually advance the Tour by calling `myTour.next()`.
* @param {function} options.beforeShowPromise A function that returns a promise.
* When the promise resolves, the rest of the `show` code for the step will execute.
* @param {Object[]} options.buttons An array of buttons to add to the step. By default
* we add a Next button which triggers `next()`, set this to `false` to disable.
BrianSipple marked this conversation as resolved.
Show resolved Hide resolved
* @param {Object[]} options.buttons An array of buttons to add to the step. These will be rendered in a
* footer below the main body text.
* @param {function} options.buttons.button.action A function executed when the button is clicked on
* @param {string} options.buttons.button.classes Extra classes to apply to the `<a>`
* @param {Object} options.buttons.button.events A hash of events to bind onto the button, for example
Expand Down Expand Up @@ -120,7 +119,7 @@ export class Step extends Evented {
* @param {HTMLElement} content The element for the step, to append the footer with buttons to
*/
_addButtons(content) {
if (this.options.buttons) {
if (!isEmpty(this.options.buttons)) {
const footer = document.createElement('footer');
const buttons = createFromHTML('<ul class="shepherd-buttons"></ul>');

Expand Down Expand Up @@ -339,8 +338,6 @@ export class Step extends Evented {
forOwn(when, (handler, event) => {
this.on(event, handler, this);
});

this._setupButtons();
BrianSipple marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -357,31 +354,6 @@ export class Step extends Evented {
this._show();
}

/**
* Determines button options prior to rendering
*
* @private
*/
_setupButtons() {
const { buttons } = this.options;
if (buttons) {
const buttonsAreDefault = isUndefined(buttons) || isEmpty(buttons);
if (buttonsAreDefault) {
this.options.buttons = [{
text: 'Next',
action: this.tour.next,
classes: 'btn'
}];
} else {
const buttonsAreObject = isPlainObject(buttons);
// Can pass in an object which will assume a single button
if (buttonsAreObject) {
this.options.buttons = [this.options.buttons];
}
}
}
}

/**
* Triggers `before-show`, generates the tooltip DOM content,
* sets up a tippy instance for the tooltip, then triggers `show`.
Expand Down
14 changes: 2 additions & 12 deletions test/cypress/integration/test.acceptance.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,10 @@ describe('Shepherd Acceptance Tests', () => {
cy.contains('Next').click();

// Step two text should be visible
cy.contains('Including Shepherd is easy!')
.should('exist')
.and('be.visible');

// Step one text should be hidden
cy.get('.shepherd-text')
.contains('Shepherd is a JavaScript library')
.contains('Including Shepherd is easy!')
.should('exist')
.and('be.hidden');
.and('be.visible');

// Click back
cy.contains('Back').click();
Expand All @@ -140,11 +135,6 @@ describe('Shepherd Acceptance Tests', () => {
.contains('Shepherd is a JavaScript library')
.should('exist')
.and('be.visible');

// Step twp text should be hidden
cy.contains('Including Shepherd is easy!')
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? We should keep the test functionality in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was wrong when I added it in the first place.

The hidden steps souldn't exist in the DOM at all. And also, Cy works in a way that it will try to wait until it finds something -- which causes it to hang here.

Copy link
Member

Choose a reason for hiding this comment

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

Unless we changed something, hidden steps definitely should exist in the DOM. Once a step is created and inserted into the DOM, it remains there, but is just set to hidden=true. In this case, it may not have been inserted into the DOM yet, but if that is the case, we should do a couple next/back clicks to make sure it is inserted into the DOM, and hidden when on another step. Not sure if we already have tests covering that or not.

Copy link
Contributor Author

@BrianSipple BrianSipple Oct 12, 2018

Choose a reason for hiding this comment

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

We set this.el to hidden, but then we also call hide on the tippy instance.

  hide() {
    this.trigger('before-hide');

    if (this.el) {
      this.el.hidden = true;
      // We need to manually set styles for < IE11 support
      this.el.style.display = 'none';
    }

    document.body.('data-shepherd-step');

    if (this.target) {
      this.target.classList.remove('shepherd-enabled', 'shepherd-target');
    }

    if (this.tooltip) {
      this.tooltip.hide();
    }

    this.trigger('hide');
  }

Since the former is contained in the later, it gets removed from the DOM

.should('exist')
.and('be.hidden');
});
});

Expand Down
17 changes: 17 additions & 0 deletions test/cypress/utils/default-buttons.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
export default {
cancel: {
classes: 'shepherd-button-secondary cancel-button',
text: 'Exit',
type: 'cancel'
},
next: {
classes: 'shepherd-button-primary next-button',
text: 'Next',
type: 'next'
},
back: {
classes: 'shepherd-button-primary back-button',
text: 'Back',
type: 'back'
}
};
58 changes: 47 additions & 11 deletions test/unit/test.step.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const { assert } = chai;
import Shepherd from '../../src/js/shepherd.js';
import { Step } from '../../src/js/step.js';
import { Tour } from '../../src/js/tour.js';
import defaultButtons from '../cypress/utils/default-buttons';

// since importing non UMD, needs assignment
window.Shepherd = Shepherd;

Expand Down Expand Up @@ -406,16 +408,6 @@ describe('Step', () => {
step.destroy();
assert.isOk(whenCalled);
});

it('sets up buttons', () => {
BrianSipple marked this conversation as resolved.
Show resolved Hide resolved
const setupButtonsSpy = spy(Step.prototype, '_setupButtons');

assert.equal(setupButtonsSpy.callCount, 0);

new Step('test', {});

assert.equal(setupButtonsSpy.callCount, 1);
});
});

describe('getTour()', () => {
Expand Down Expand Up @@ -446,7 +438,7 @@ describe('Step', () => {
step._addContent(content);
assert.equal(content.querySelectorAll('.shepherd-text p').length, 2);
assert.equal(step.options.text.join(' '),
Array.from(content.querySelectorAll('.shepherd-text p')).map((text) => text.innerHTML).join(' '));
Array.from(content.querySelectorAll('.shepherd-text p')).map((text) => text.innerHTML).join(' '));
});

it('applies HTML element directly to content', () => {
Expand All @@ -472,4 +464,48 @@ describe('Step', () => {
assert.equal('I am some test text.', content.querySelector('.shepherd-text p').innerHTML);
});
});

describe('_addButtons', () => {
it('renders no buttons if an empty array is passed to `options.buttons`', () => {
const content = document.createElement('div');
const step = new Step();

step.options.buttons = [];

step._addButtons(content);

assert.equal(content.children.length, 0);
});

it('renders no buttons if nothing is passed to `options.buttons`', () => {
const content = document.createElement('div');
const step = new Step();

step._addButtons(content);

assert.equal(content.children.length, 0);
});

it('renders buttons for each item passed to `options.buttons`', () => {
const content = document.createElement('div');
const step = new Step();

step.options.buttons = [
defaultButtons.cancel,
defaultButtons.next,
];

step._addButtons(content);

assert.equal(content.children.length, 1);

const buttonContainer = content.querySelector('.shepherd-buttons');

assert.equal(buttonContainer instanceof HTMLElement, true);

const buttons = buttonContainer.querySelectorAll('.shepherd-button');

assert.equal(buttons.length, 2);
});
});
});