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

Feature: Setting the start position #21

Closed
wants to merge 4 commits into from
Closed

Feature: Setting the start position #21

wants to merge 4 commits into from

Conversation

spaluchiewicz
Copy link

This PR is to solve #20 (Setting the start position)

@HamzaGhazouani HamzaGhazouani self-requested a review September 18, 2017 21:08
@spaluchiewicz
Copy link
Author

@HamzaGhazouani will you be able to review and approve this PR?

@HamzaGhazouani
Copy link
Owner

Hi @spaluchiewicz, thanks for your contribution, I will look at it soon, it will be nice if we add an example of this feature in the sample

@spaluchiewicz
Copy link
Author

Sure, I will update the PR.

Copy link
Owner

@HamzaGhazouani HamzaGhazouani left a comment

Choose a reason for hiding this comment

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

Hi @spaluchiewicz,

Thanks for your contribution.
We should do some changes before the merge:

  • Add documentation to circleInitialAngle property
  • Add documentation and explanation to func rotate(around center: CGPoint, with degrees: CGFloat), the second parameter is in radian and not degrees.
  • We should not modify the CircularSliderHelper.circleInitialAngle because it can be used by other instances of circular slider and we will get errors (we should remove it and use an instance property instead)

For the example, we can just change the initial angle of the last example (circular) in the tab bar

Thanks

HamzaGhazouani
HamzaGhazouani previously approved these changes Oct 6, 2017
@HamzaGhazouani HamzaGhazouani dismissed their stale review October 6, 2017 09:20

Approved by mistake

@iblagajic
Copy link

Hi, can I help wrap this up?

@HamzaGhazouani
Copy link
Owner

Hi @iblagajic,
you can checkout @spaluchiewicz branch's, and if you can do the modifications in my comment above it will be perfect, thanks a lot

@iblagajic
Copy link

Should I create a new PR or can I contribute to the existing one?

@HamzaGhazouani
Copy link
Owner

@iblagajic as you prefer :)

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

Successfully merging this pull request may close these issues.

3 participants