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

Update MMScrollPresenter.m #4

Closed
wants to merge 1 commit into from
Closed

Conversation

nwpuhmz
Copy link
Contributor

@nwpuhmz nwpuhmz commented Apr 16, 2015

Hi MitchellMalleo , i updated the file ,adding an UIPageControl .Maybe we should let the user make a decision whether to use the UIPageControl ,but now it's hard -coded .

Add UIPageControl
@MitchellMalleo
Copy link
Owner

Hey nwpuhmz,

A few things to comment for this pull request. I like the idea giving the user an option to have a UIPageControl, but there are some issues. Its placed awkwardly in-between the titles which adversely effects readability, having it hard-coded isn't ideal; there should be a BOOL named shouldShowPageControl or something along those lines, and the control gives full access to the pageArray so the developer could implement their own UIPageControl and use the addSubview method provided. I'd be more open to a custom UIPageControl that fits the design of the MMScrollPresenter.

Also its generally discouraged to have abbreviated naming conventions in obj-c code. Check out this style guide: https://github.com/raywenderlich/objective-c-style-guide

@nwpuhmz
Copy link
Contributor Author

nwpuhmz commented Apr 16, 2015

Nice advice !^_^

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.

None yet

2 participants