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

Stop disabling target element pointer events #245

Merged
merged 1 commit into from
Sep 13, 2018

Conversation

BrianSipple
Copy link
Contributor

@BrianSipple BrianSipple commented Sep 12, 2018

I got started on a branch in shepherd that will allow for a disableAttachTarget step option.

In the meantime, though, we can get started with the corresponding change here.

@BrianSipple BrianSipple force-pushed the stop-disabling-step-element-pointer-events branch from c98732f to 76bba0d Compare September 12, 2018 06:32
@BrianSipple
Copy link
Contributor Author

Side note: Whenever modal mode is active, target elements won't be clickable anyway. So I think it works out quite nicely that we can have that broad experience here, while still enabling fine-grained step control at the shepherd level.

Copy link
Owner

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

Remember we were not going to just remove this stuff, but allow a clickable: false to be passed to enable it?

@BrianSipple
Copy link
Contributor Author

Okay, I was experimenting with the shepherd approach and figured that since clickable would be true by default, this would be good start.

I can refactor this, though, after some of our discussion about keeping things in ember-shepherd

@BrianSipple
Copy link
Contributor Author

BrianSipple commented Sep 12, 2018

@rwwagner90 @chuckcarpenter Thinking about how to document this... this might be a good opportunity to clean up the README and only document the extra step options (while linking to docs for Shepherd's step options.

Also, what you think about isClickable (as opposed to clickable)?

@BrianSipple
Copy link
Contributor Author

I still kind of like disableAttachTarget, too. clickable might be confusing, because I can imagine a lot of people thinking it refers to the step element itself.

@RobbieTheWagner
Copy link
Owner

this might be a good opportunity to clean up the README and only document the extra step options

Let's keep all the docs for now.

I still kind of like disableAttachTarget, too. clickable might be confusing, because I can imagine a lot of people thinking it refers to the step element itself.

I'd like a simpler name. clickable I think makes sense to the masses. disableAttachTarget isn't clear to me what it does. I'm open to other names, but I want it to be something the common person can understand. Perhaps attachToElementClickable, interactiveAttachTo, etc? I'm not sure of a good name myself. It's going to have to be documented regardless, so the name isn't super important.

@BrianSipple
Copy link
Contributor Author

canClickTarget?

@RobbieTheWagner
Copy link
Owner

I think I'm okay with canClickTarget. Let's just get something in there, and see if it feels right 😄

@BrianSipple BrianSipple force-pushed the stop-disabling-step-element-pointer-events branch from 76bba0d to 1864d8d Compare September 12, 2018 22:30
@BrianSipple
Copy link
Contributor Author

@rwwagner90 Just updated and rebased around implementing a canClickTarget step option.

Copy link
Owner

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

We've got some code climate issues, but we can fix those later. Thanks!

@RobbieTheWagner RobbieTheWagner merged commit af93faa into master Sep 13, 2018
@RobbieTheWagner RobbieTheWagner deleted the stop-disabling-step-element-pointer-events branch September 13, 2018 01:00
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 this pull request may close these issues.

2 participants