-
Notifications
You must be signed in to change notification settings - Fork 45
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
Change builtInButtons to buttons #241
Conversation
@chuckcarpenter I don't think we should call something that's not the same as Shepherd, the same as Shepherd here. |
I'm still in favor of merging |
It seems like the primary issue here is that the interface lacks constraint. What if we had two separate options: Each is focused, explicit, and unambiguous — for us and for users... unlike |
@BrianSipple I would keep We could potentially use just |
Right, isn't that what we're trying to enable? The current confusion is from the fact that Or am I confused about what we're confused about? (#224) |
@BrianSipple Shepherd does not provide any buttons. Shepherd takes an object to construct buttons, but has no built in or default types. ember-shepherd, however, does provide built in buttons, so you just set With this in mind, I think perhaps it would make sense to just use Would this work for you @chuckcarpenter? |
Ah, got it. I'm leaning towards using Because that's actually the only difference between So I think... a) keeping the interface consistent while... b) documenting the extra sugar... strikes a pretty good mix. |
Totally agree @BrianSipple! We just need to fix conflicts here and add the |
e509bd1
to
e6e0fc7
Compare
c5b8d2a
to
e94d798
Compare
This branch obfuscates the binding for buttons in steps to the user and makes setup mirror the docs from Shepherd.js.
Resolves #224