[iOS] Add session info to Speaker View #633
Conversation
| let sessions: [Session] = self.items.filter { session in | ||
| if let speechSession = session as? SpeechSession { | ||
| return speechSession.speakers.contains { $0.id == speaker.id } | ||
| } else { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this part has a bug.
If this goes on, speaker view's session information shows sessions which is shown in this tab.
I think the bug part is occurred in MY PLAN tab. We should fetch data in SpeakerViewController 's ViewModel based on speakerID.
ちょっと英語で伝え切れるかわからないので日本語でも書きます。
このままだと スピーカービューの セッション情報に表示される内容はタップしたセッションのあるタブ内のセッションからしか表示されないと思います。
このバグが起こる例としては MY PLAN タブです。（検索の画面とかでも使いたいのでそこではそもそもうまく使えなくなってしまうかと思います。）
解決策として考えられるのは、SpeakerViewController に対応した ViewModel を作成し、そこでスピーカーのセッション情報を取得することです。
長くなりました。修正お願いします 🙏
There was a problem hiding this comment.
@ry-itto
I see.
I will do other PR, so please merge this one.
There was a problem hiding this comment.
I think we don't need to split PR for this. Would you fix this in same PR ?
There was a problem hiding this comment.
I was really upset to fix many conflicts during previous my PR... I don't want to waste time to such a things, so I want to merge early timing😞
There was a problem hiding this comment.
@takahirom
What should I do about this?
I think above should fix in this PR.
Please let me know your opinion 🙏
There was a problem hiding this comment.
@ry-itto
Hi, I'm DroidKaigi member too.
I think it's okay to merge and create a new PR 😄
Because MY PLAN has not been still completed.
Even if it has unresolved problem, it is recommended that would you create a small PR to easy merging.
There was a problem hiding this comment.
@roana0229
Hi, thank you for replying 😄
I see.
Even if it has unresolved problem, it is recommended that would you create a small PR to easy merging.
I understood. For repositories with fast implementation speed, I certainly felt that it was better.
I will merge this. 👍
|
Your apk has been deployed to https://deploygate.com/distributions/86b6ec8c73dc9e3a75a8b6dc5ead2acb1fea1588. Anyone can try your changes via the link. Generated by 🚫 Danger |
|
No issue was reported. Cool! Generated by 🚫 Danger |
Issue
Overview (Required)
Links
Screenshot