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

[WIP] Popper v2 #752

Merged
merged 15 commits into from
Feb 12, 2020
Merged

[WIP] Popper v2 #752

merged 15 commits into from
Feb 12, 2020

Conversation

RobbieTheWagner
Copy link
Member

@RobbieTheWagner RobbieTheWagner commented Jan 8, 2020

Giving Popper v2 a try, in the hopes that it is smaller and performs better than Tether.

  • Convert Modifiers to new format
  • Replace tests with Popper centric ones
  • Update docs to account for v2 syntax
  • Fix eslint in rollup

src/js/step.js Outdated Show resolved Hide resolved
@atomiks
Copy link

atomiks commented Jan 9, 2020

The eslint problem is a bug with the plugin: TrySound/rollup-plugin-eslint#40

Downgrade to 6.0.0 and it will work apparently

@RobbieTheWagner
Copy link
Member Author

@atomiks downgrading throws other errors. We can keep it commented out for now.

src/js/components/shepherd-element.svelte Outdated Show resolved Hide resolved
src/js/utils/general.js Outdated Show resolved Hide resolved
test/cypress/integration/a11y.spec.js Show resolved Hide resolved
expect(tetherOptions.classes.element).toBe('bar');
});

it('constraints can be overridden', function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we replace these tests with some popperOptions tests? Would be good to make sure deeply nested options can be passed and override ours. I had issues with this in the past.

Copy link
Member

Choose a reason for hiding this comment

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

I was a little back and forth on this, but should it be on the implementor to understand what they're passing in and use v2 docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should add tests for the options merging is what I mean. like options.modifiers.foo.bar and make sure that overrides our defaults and can still be passed down, and we don't clobber it. Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a lot of issues before with using Object.assign and accidentally clobbering nested options.

[`appendTo` option](https://atomiks.github.io/tippyjs/#append-to-option) by defining it on the
`tippyOptions` hash inside of each Step's options hash.
By default, tour steps will append their elements to the `body` element of the DOM. This is perfect for most use cases, but not always. If you need to have steps appended elsewhere you can take advantage of Popper's
[options](https://popper.js.org/docs/v2/constructors/#options) by defining it on the
Copy link
Member Author

Choose a reason for hiding this comment

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

What popper option would be used for this? We may want to be explicit here.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is a tippy option and we easily do this with Step.target. Going to remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think step.target is the same thing. This is defining the container the step element itself lives in right?

Copy link
Member

Choose a reason for hiding this comment

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

if you look at the docs on createPopper() The first arg is target and we map target there. Maybe I'm not clear on what you mean then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Target is what you are attaching the popper to. appendTo is where the tooltip element itself lives.

@RobbieTheWagner
Copy link
Member Author

@chuckcarpenter I think the only thing missing from this PR is constraining to window. i.e. when the window shrinks and there is no room to flip to the other side, the tooltips should start moving to the middle of the target, to allow them to be read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants