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 message-only coachmarks #1

Closed
kaandedeoglu opened this issue Oct 23, 2015 · 14 comments
Closed

Show message-only coachmarks #1

kaandedeoglu opened this issue Oct 23, 2015 · 14 comments

Comments

@kaandedeoglu
Copy link

Sometimes it is preferable to display just a text, without a next button or a separator. It seems Instructions does not support this at the moment. But I think it would be a good addition.

@ephread
Copy link
Owner

ephread commented Oct 23, 2015

It supports message-only coach marks, if you provide a custom view. :) I don't really want to go too far with the “default” coach marks, since everyone has different needs.

@kaandedeoglu
Copy link
Author

fair enough :) I'd like to send a pull request for this if you're not completely against the idea.

@ephread
Copy link
Owner

ephread commented Oct 23, 2015

I'm not completely against the idea. ;) How do you envision this? Another CoachMarkBodyDefaultView-like class with only text in it?

Do you think the “default” views are really usable “as-is”? I actually never thought anyone would use the “default” styles!

@kaandedeoglu
Copy link
Author

Yes why not, most people just want basic views as long as they are somewhat configurable.

Things would get a lot more flexible if the hintText and nextText were given in the constructor. like

coachMarksController.defaultCoachViewsWithArrow(true, arrowOrientation: coachMark.arrowOrientation, hintText: String, nextText: String?)

and the separatorView and the nextLabel were added iff nextText is non-nil.

Of course, thinking about it now, it all takes 1 subclass similar to the CoachMarkBodyDefaultView per project. So what you did is totally valid as well :)

@ephread
Copy link
Owner

ephread commented Oct 23, 2015

I guess giving a little more flexibility to the “default” coach mark is okay. :) Go for it!

@ephread
Copy link
Owner

ephread commented Oct 31, 2015

@kaandedeoglu any updates on that pull request? :)

@kaandedeoglu
Copy link
Author

@ephread sorry have been super busy - I've already implemented something similar for my own use case, but it's not generic enough yet. Will update you on it when I get a chance. I can close this for the time being if your prefer.

@ephread
Copy link
Owner

ephread commented Nov 2, 2015

Alright, we're all busy, aren't we :). That's no big deal, the issue can remain open, we'll close it once the pull request is ready. I simply wanted to make sure the feature was still in the pipeline!

estebansotoara added a commit to estebansotoara/Instructions that referenced this issue Nov 12, 2015
@estebansotoara
Copy link
Contributor

Hi
I saw this issue open and decided to give a hand, I added the constructor
defaultCoachViewsWithArrow(withArrow: Bool = true, arrowOrientation: CoachMarkArrowOrientation? = .Top, hintText: String, nextText: String?)
as kaandedeoglu suggested and follow the same rule, only shows the nextLabel and separatorView if nextText is provided.

I leave the original
defaultCoachViewsWithArrow(withArrow: Bool = true, arrowOrientation: CoachMarkArrowOrientation? = .Top)
in case anyone want or need to set the hintText and nextText later (also for retrocompatibility)

@ephread I have a fork with the changes (https://github.com/estebansotoara/Instructions/tree/ShowMessageOnlyCoachmarks), you can check it out and if you like it I can make a PR

@ephread
Copy link
Owner

ephread commented Nov 13, 2015

Thanks @estebansotoara! I'll take a look in the next few days. ;)

@ephread
Copy link
Owner

ephread commented Dec 11, 2015

@estebansotoara sorry for the HUGE delay, it looks mostly good ;), can you start a PR, so I can make few suggestions on the related issue?

@estebansotoara
Copy link
Contributor

Done, the PR is #18

@hardikdevios
Copy link

please merge this PR and update the podspec

@ephread
Copy link
Owner

ephread commented Feb 15, 2016

PR #18 was merged.

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

No branches or pull requests

4 participants