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.

Sign up for GitHub

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

Jump to bottom

increase touch area if checkbox is smaller than 44pt #19

Merged
merged 3 commits into from
Apr 14, 2016
Merged

increase touch area if checkbox is smaller than 44pt #19

merged 3 commits into from
Apr 14, 2016

Conversation

xinsight
Copy link

If the checkbox is small, it can be difficult to tap. This PR increases the hit test area to a minimum of 44pt.

@Boris-Em
Copy link
Owner

Boris-Em commented Apr 14, 2016 •
edited
Loading

Hi @xinsight,
Thanks a lot for your work!
I think that the current implementation you propose is too limiting. For example, I could see cases, where increasing the hit area is not desired.
To allow more flexibility, I would expose publicly minimumSize as a property for example. Giving it a default value of 44.0, and a more explicit name.
I also left a few minor in line comments.
Thanks again for the PR!

Boris-Em
Boris-Em reviewed Apr 14, 2016
View reviewed changes
Classes/BEMCheckBox.m
BOOL found = [super pointInside:point withEvent:event];

CGFloat minimumSize = 44;
CGFloat w = self.frame.size.width;
Copy link
Owner

@Boris-Em Boris-Em Apr 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

To conform with the rest of the variable names in the project, please use the full word "width".
Shouldn't we also use bounds instead of frame here?

@xinsight
Copy link
Author

Thanks for the feedback. 44pt is Apple's recommended value, so i'm not sure why someone would want it smaller, but sure, i can make it configurable.

@Boris-Em
Copy link
Owner

Thanks a lot @xinsight, great work!

@Boris-Em Boris-Em merged commit 257da07 into Boris-Em:master Apr 14, 2016
@xinsight xinsight mentioned this pull request Apr 15, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xinsight @Boris-Em