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/create chat room vc #29

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Feature/create chat room vc #29

wants to merge 34 commits into from

Conversation

ojun9
Copy link
Contributor

@ojun9 ojun9 commented Jul 19, 2020

概要

チャット選択画面を追加した

レビュー観点

コードの書き方
collectionviewとtableviewが同様のデータを持っているが表示データ自体を変数で分けて良いのか

レビューレベル

  • Lv0: まったく見ないでAcceptする
  • Lv1: ぱっとみて違和感がないかチェックしてAcceptする
  • Lv2: 仕様レベルまで理解して、仕様通りに動くかある程度検証してAcceptする
  • Lv3: 実際に環境で動作確認したうえでAcceptする

スクリーンショット

ChatsRoom

備考

ユーザ検索は全文一致で部分文字列の検索はしてない
プロフィール画像の表示はしてない
チャットルームをfirestoreに保存はしてない

ojun9 added 24 commits July 18, 2020 11:38
Copy link
Contributor

@shinya-todaka shinya-todaka left a comment

Choose a reason for hiding this comment

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

レビューしました!
自分も勉強のために全く同じものを作ってみたのですが
https://github.com/marty-suzuki/iOSDesignPatternSamples/tree/mvp/iOSDesignPatternSamples/Sources/UI/Favorite
が参考になりました。
あと自分もしっかり考えたことなかったですがclassのプロパティとメソッドへのアクセスはselfは省略した方が良いかもです。
参考 https://qiita.com/taketomato/items/d5630fe5df45f4093720#3-classes

Comment on lines 218 to 226
cell.deleteUserButton.tag = indexPath.item
cell.deleteUserButton.addTarget(self, action: #selector(tapSelectedUserCollectionViewCellDeleteUserButton(_:)), for: .touchUpInside)

//TODO:Firestoreから取得した後で表示し直すこと
if #available(iOS 13.0, *) {
cell.profileImageView.image = UIImage(systemName: "bolt.circle.fill")
} else {
// Fallback on earlier versions
}
Copy link
Contributor

Choose a reason for hiding this comment

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

tagを使わずにdelegateかクロージャの方が良いかなと思うのですがどうでしょうか?
参考 https://fluffy.es/handling-button-tap-inside-uitableviewcell-without-using-tag/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

これは結局SearchUserTableviewCell内に
cell番号自体を保存する必要がある(サイト内ではユーチューバの名前がクロージャーで送られてる)かつ,ボタンで使うのはcellで固有のString等の情報でなく単にcellの番号のみの取得なので,今回の処理ではtagを使った実装にしたのですがどうでしょうか?

Copy link
Contributor

Choose a reason for hiding this comment

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

タグを使わない方が良い(例えばsectionがいっこだけじゃない時にややこしくなる)と思っていて
その上で今回はtagを使ってindexPath.itemを渡しているので
それならクロージャを使って
cellのクラス内でbuttonがtapされた時に
deleteUserActionが実行されるようにして

cell.deleteUserAction = { [weak self] in 
   self?.presenter.didTapSelectedUserCollectionViewCellDeleteUserButton(index: indexPath.item)
}

こんな感じでindexPath.itemを渡した方が良いかなと思いました

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほど。理解しました!了解です
クロージャで実装します!

@ojun9
Copy link
Contributor Author

ojun9 commented Jul 22, 2020

修正しました!

@ojun9
Copy link
Contributor Author

ojun9 commented Jul 23, 2020

Room保存機能を実装した上でcellのボタンがタップされた処理を修正しました

@shinya-todaka
Copy link
Contributor

shinya-todaka commented Jul 23, 2020

いいと思います!

    var searchedUsers: [User] {
        return model.searchedUsersArray
    }
    
    var selectedUsers: [User] {
        return model.selectedUsersArray
    }
    
    func isSelected(user: User) -> Bool {
        return selectedUsers.firstIndex { user.id == $0.id } != nil
    }

presenterにこんな感じで書いたら

    func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
        guard let cell = self.serchUserTableview.dequeueReusableCell(withIdentifier: self.searchedUsersCellID, for: indexPath)
                         as? SearchUserTableviewCell else { return UITableViewCell() }
        let user = presenter.searchedUsers[indexPath.item]
        let isSelected = presenter.isSelected(user: user)
        cell.configure(with: user, isSelected: isSelected)
        return cell
    }

cellのとこがいい感じになると思います。(modelのisContainsも消せますね)
collectionViewのcell のところも

    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
        let cell = collectionView.dequeueReusableCell(withReuseIdentifier: selectedUsersCellID, for: indexPath) as! SelectedUserCollectionViewCell
        cell.configure(with: presenter.selectedUsers[indexPath.item])
        cell.deleteUserButtonAction = { [weak self] in
            self?.presenter.didTapSelectedUserCollectionViewCellDeleteUserButton(index: indexPath.item)
        }
        return cell
    }

tableViewとcollectionViewのcellの初期化はviewに値を渡すところは全部configureでして
こんな感じでした方が綺麗かもしれません!
自分が気になっただけなのでここら辺はお好みでお願いします!

あとroomNameは
Bob,二人目の名前,三人めの名前としたいのなら
let roomName = ( [name] + selectedUsersArray.compactMap { $0.displayName }).joined(separator: ",")
こうですかね。

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.

2 participants