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

🐛 Fix initial scroll to top before scrolling to target #801

Merged
merged 4 commits into from
Feb 20, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 6 additions & 0 deletions src/js/components/shepherd-element.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,12 @@
opacity: 1;
}

.shepherd-element[data-popper-reference-hidden]:not(.shepherd-centered) {
visibility: hidden;
pointer-events: none;
opacity: 0;
}

.shepherd-element, .shepherd-element *,
.shepherd-element *:after,
.shepherd-element *:before {
Expand Down
18 changes: 8 additions & 10 deletions src/js/step.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import ShepherdElement from './components/shepherd-element.svelte';

// Polyfills
import smoothscroll from 'smoothscroll-polyfill';

smoothscroll.polyfill();

/**
Expand Down Expand Up @@ -329,7 +328,6 @@ export class Step extends Evented {
if (this.options.advanceOn) {
bindAdvance(this);
}

setupTooltip(this);
}

Expand All @@ -345,22 +343,22 @@ export class Step extends Evented {

this.tour.modal.setupForStep(this);
this._styleTargetElementForStep(this);

this.el.hidden = false;

this.tooltip.update();
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we can remove this? Perhaps this was the wrong approach, but I think we needed to call update to ensure things got repositioned when scrolling or resizing the page etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

this seems to do extra renders, does the eventlistener for scroll call the initial show function?

Copy link
Member

Choose a reason for hiding this comment

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

Unsure, but we need an update somewhere I think. Maybe I am wrong and popper now handles this internally?

Copy link
Member

Choose a reason for hiding this comment

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

Does it update on scroll and resize without this?

Copy link
Member Author

Choose a reason for hiding this comment

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

when it is out of viewport, there's a data attr added and we use that to set opacity to 0. What behavior where you expecting. Also, resize seems to have it's own internal events, as it does work as I'd expect.

Copy link
Member

Choose a reason for hiding this comment

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

Just ensuring that the popper moves around and tries to stay in the viewport. I think on resize and scroll, the popper would stay in one spot before, if we weren't calling update.

Copy link
Member

Choose a reason for hiding this comment

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

I checked this out locally, and it seems to be fine 👍


const content = this.shepherdElementComponent.getElement();
const target = this.target || document.body;
target.classList.add(`${this.classPrefix}shepherd-enabled`, `${this.classPrefix}shepherd-target`);
content.classList.add('shepherd-enabled');

// start scrolling to target before showing the step
if (this.options.scrollTo) {
setTimeout(() => {
this._scrollTo(this.options.scrollTo);
});
}

this.el.hidden = false;

const content = this.shepherdElementComponent.getElement();
const target = this.target || document.body;
target.classList.add(`${this.classPrefix}shepherd-enabled`, `${this.classPrefix}shepherd-target`);
content.classList.add('shepherd-enabled');

this.trigger('show');
this.el.focus();
}
Expand Down
54 changes: 53 additions & 1 deletion src/js/utils/general.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,57 @@
import { createPopper } from '@popperjs/core';
import { isString } from './type-check';
import { makeCenteredPopper } from './popper-options';
import getNodeName from '@popperjs/core/lib/dom-utils/getNodeName.js';
import { isHTMLElement } from '@popperjs/core/lib/dom-utils/instanceOf.js';
import applyStylesModifier from '@popperjs/core/lib/modifiers/applyStyles';

function effectFixed({ state }) {
const initialStyles = {
position: 'absolute',
left: '0',
top: '0',
margin: '0'
};

Object.assign(state.elements.popper.style, initialStyles);

return () => {
Object.keys(state.elements).forEach((name) => {
const element = state.elements[name];
const styleProperties = Object.keys(
Object.prototype.hasOwnProperty.call(state.styles, name)
? { ...state.styles[name] }
: initialStyles
);
const attributes = state.attributes[name] || {};

// Set all values to an empty string to unset them
const style = styleProperties.reduce(
(style, property) => ({
...style,
[String(property)]: ''
}),
{}
);

// arrow is optional + virtual elements
if (!isHTMLElement(element) || !getNodeName(element)) {
return;
}

// Flow doesn't support to extend this property, but it's the most
// effective way to apply styles to an HTMLElement
// $FlowFixMe
Object.assign(element.style, style);

Object.keys(attributes).forEach((attribute) =>
element.removeAttribute(attribute)
);
});
};
}
// work around for default state values
applyStylesModifier.effect = effectFixed;

/**
* Ensure class prefix ends in `-`
Expand Down Expand Up @@ -83,8 +134,9 @@ export function uuid() {
*/
export function getPopperOptions(attachToOptions, step) {
let popperOptions = {
// required to keep the Step in the viewport
modifiers: [
applyStylesModifier,
// required to keep the Step in the viewport
{
name: 'preventOverflow',
options: {
Expand Down
6 changes: 3 additions & 3 deletions src/js/utils/popper-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,10 @@ function _getCenteredStylePopperModifier() {
*/
export function makeCenteredPopper(step) {
const centeredStylePopperModifier = _getCenteredStylePopperModifier(step);
const content = step.shepherdElementComponent.getElement();
let popperOptions = _makeCommonPopperOptions(step);

popperOptions.placement = 'top';
popperOptions.strategy = 'absolute';

content.classList.add('shepherd-centered');
popperOptions = {
...popperOptions,
modifiers: Array.from(new Set([...popperOptions.modifiers, ...centeredStylePopperModifier]))
Expand All @@ -62,6 +61,7 @@ export function makeCenteredPopper(step) {

function _makeCommonPopperOptions() {
const popperOptions = {
placement: 'top',
strategy: 'fixed',
modifiers: []
};
Expand Down