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

Created Yoshi #1

Merged
merged 23 commits into from
Feb 8, 2016
Merged

Created Yoshi #1

merged 23 commits into from
Feb 8, 2016

Conversation

MichaelCampbell
Copy link
Contributor

@quentinR @yoyoyoseob @htinlinn @paulmiard please review

  • Added YoshiCustomMenu implementation
  • Added YoshiMenuDateSelector implementation
  • Added Labels to ViewController so that we can see when the selections are updated.

@MichaelCampbell MichaelCampbell self-assigned this Jan 5, 2016
let menuItemQA = MenuItem(name: "QA")
let environmentItems = [menuItemProd, menuItemStaging, menuItemQA].map { $0 as YoshiTableViewMenuItem }

let tableViewMenu = TableViewMenu(debugMenuName: "Environment", menuType: .TableView, displayItems: environmentItems, didSelectDisplayItem: { (displayItem) in

Choose a reason for hiding this comment

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

why do you need to specify the menu type when you use TableViewMenu? you should default here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are three different types of menu items

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the type

Choose a reason for hiding this comment

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

Yes but when you create a TableViewMenu the menu type should be defaulted to table view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitely

- parameter event: (UIEvent) the event captured by the original motionBegan call
- parameter minimumTouchRequirement: (Int) the minimum number of touches required to show the debug menu.
*/
public class func touchesBegan(touches: Set<UITouch>, withEvent event: UIEvent?, minimumTouchRequirement: Int = 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

DOPE!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😎

@paulmiard
Copy link
Contributor

Any chance you can make it iOS 8 compliant? Or maybe in a next PR?

@MichaelCampbell
Copy link
Contributor Author

just logged #2 @paulmiard. I'll take of this in the next PR. This one has already become pretty large


override func viewDidLoad() {
super.viewDidLoad()
self.setupDatePicker()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should self be used here? I was under the impression that self was only really required within closures

Wait, the PiOS Swift Style guide still says
Use of self Please use self, even when it's not necessary, to make the property's owner obvious. (Note: this disagrees with the RW style guide)
https://bitbucket.org/prolificinteractive/pios/src/master/Coding%20Conventions/SwiftStyleGuide.md?fileviewer=file-view-default

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use self.

Also don't use that style guide. It's out of date and has been moved: https://github.com/prolificinteractive/swift-style-guide

@paulmiard
Copy link
Contributor

👍

MichaelCampbell added a commit that referenced this pull request Feb 8, 2016
@MichaelCampbell MichaelCampbell merged commit fcb36b1 into develop Feb 8, 2016
@MichaelCampbell MichaelCampbell deleted the feature/create_pod branch May 24, 2016 00:21
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.

7 participants