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

Show event of tour does not pass the hash of step and previous #371

Closed
hodossy opened this issue May 14, 2019 · 7 comments · Fixed by #381
Closed

Show event of tour does not pass the hash of step and previous #371

hodossy opened this issue May 14, 2019 · 7 comments · Fixed by #381
Labels

Comments

@hodossy
Copy link

hodossy commented May 14, 2019

I am using Shepherd 2.8.0. The problem is in trigger where the arguments is passed to drop, since arguments is not an Array, therefore always an empty list is returned and passed to the show handler as args.

An easy fix would be to convert to array before passing to drop:

trigger(event) {
    if (!isUndefined(this.bindings) && this.bindings[event]) {
      const args = drop(Array.prototype.slice.call(arguments));

      this.bindings[event].forEach((binding, index) => {
        const { ctx, handler, once } = binding;

        const context = ctx || this;

        handler.apply(context, args);

        if (once) {
          this.bindings[event].splice(index, 1);
        }
      });
}
@RobbieTheWagner
Copy link
Member

Hello @hodossy, it appears the links you posted link to a bunch of old issues, not to lines in the code.

I recently switched from using drop from lodash, to making it an internal util. Perhaps the lodash one handled when things were not arrays? I think we should fix this by modifying the drop util to do what lodash did, assuming it worked for you before with lodash?

If that sounds reasonable, and you would be comfortable submitting a PR for the change, that would be awesome!

@RobbieTheWagner
Copy link
Member

@chuckcarpenter @jaredgalanis if either of you have some time to look into this, let me know as well.

@hodossy
Copy link
Author

hodossy commented May 20, 2019

Hello @rwwagner90, I have fixed the links, sorry for that! I wanted to submit a PR, but I had trouble setting up the dev env on Windows (build command fails due to rm is not know by windows, but I have the windows build tools installed). The corrections are working for me as shown in my prev post.

@RobbieTheWagner
Copy link
Member

@hodossy can we please fix it with my suggestion above?

I recently switched from using drop from lodash, to making it an internal util. Perhaps the lodash one handled when things were not arrays? I think we should fix this by modifying the drop util to do what lodash did, assuming it worked for you before with lodash?

As for the rm issue. We could install rimraf or cross-env as a devDep, and use instead of rm.

@hodossy
Copy link
Author

hodossy commented May 22, 2019

Lodash basicaly reimplemented slice, so if you want that, then another function needs to be ported.

@RobbieTheWagner
Copy link
Member

@hodossy I am just asking if we could please make the changes to fix things inside the drop function. Perhaps passing just arguments to drop and doing Array.prototype.slice.call(arguments) inside drop etc. I am not 100% sure what would be best, but what I am going for here is to fix things inside drop, rather than trigger.

@RobbieTheWagner
Copy link
Member

It appears the only thing currently using drop is trigger, so I am going to proceed with the solution you recommended. Thanks so much @hodossy!

RobbieTheWagner added a commit that referenced this issue May 26, 2019
When we switched our `drop` implementation away from lodash, we accidentally stopped passing things like `step` and `previous` down to subsequent trigger calls. This fixes the `drop` call by ensuring `arguments` is now an array.

Fixes #371
RobbieTheWagner added a commit that referenced this issue May 26, 2019
When we switched our `drop` implementation away from lodash, we accidentally stopped passing things like `step` and `previous` down to subsequent trigger calls. This fixes the `drop` call by ensuring `arguments` is now an array.

Fixes #371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants