Estimote / iOS-Fleet-Management-SDK

Estimote Fleet Management SDK for iOS
https://developer.estimote.com
MIT License
1.18k stars 398 forks source link

Implemented image selection and transitions. #242

Closed daverPL closed 7 years ago

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 21 at r1 (raw file):

    }
    var selectedEmoji: String? {
        didSet {

we don't need empty didSet


Comments from Reviewable

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 44 at r1 (raw file):


    func updateEmoji() {
        // do the magic!

"TODO: ..." to be more visible, bro :P


Comments from Reviewable

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 45 at r1 (raw file):

    func updateEmoji() {
        // do the magic!
        finishUpdate()

self self self


Comments from Reviewable

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 49 at r1 (raw file):


    func finishUpdate() {
        navigationController?.popViewController(animated: true)

self


Comments from Reviewable

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 57 at r1 (raw file):

        super.viewDidLoad()

        self.initialEmoji = self.selectedEmoji

I think it would be better to set the initialEmoji on prepareforsegue when transitioning from NearestEmoji to ChangeEmoji screen. Then, the assign you're doing here would be unnecessary. What do you say?


Comments from Reviewable

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 127 at r1 (raw file):

        let emoji = sender.view as! UILabel
        self.selectedEmoji = emoji.text
        self.refreshLabelStates()

how about calling it in the didSet of selectedEmoji property?


Comments from Reviewable

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 113 at r1 (raw file):

    }

    func refreshLabelStates()

how about updateEmojiSelection()? we don't deal with any "states" here, do we?


Comments from Reviewable

przemekblasiak commented 7 years ago

Examples/swift/EmojiAdvertiser/EmojiAdvertiser/ChangeEmojiVC.swift, line 129 at r1 (raw file):

        self.refreshLabelStates()

        if self.selectedEmoji == self.initialEmoji {

you could add a comment to this block of code, to clarify what we're doing here and why (in 1 sentence)


Comments from Reviewable

przemekblasiak commented 7 years ago

Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

przemekblasiak commented 7 years ago

Reviewed 2 of 2 files at r2. Review status: 2 of 3 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

przemekblasiak commented 7 years ago

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable