-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix case of bot icons are invisible #107
Conversation
internal/slacklog/generator.go
Outdated
@@ -208,19 +208,22 @@ func (g *HTMLGenerator) generateMessageDir(channel Channel, key MessageMonthKey, | |||
return g.c.escapeSpecialChars(g.s.GetDisplayNameByUserID(msg.User)) | |||
}, | |||
"userIconUrl": func(msg *Message) string { | |||
var user *User |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
early return ごり押しだと
!msg.IsVisible()
であればreturn "" // TODO show default icon
- Bot からのメッセージで
Icons
が存在すればreturn msg.Icons.Image48
- 条件式が汚なくなりますが
msg.Subtype == "bot_message" || msg.Subtype == "slackbot_response"
をIsBotMessage()
とか名付ければ条件式もまだましになる...?
- 条件式が汚なくなりますが
- それ以外の場合は user を取得して存在すれば
return user.Profile.Image48
とできそうです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ただの提案(? 思い付いたことをつぶやいただけとも言う...)なので、このままでも問題自体はないと思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
書いててすっきりしなかったんですよねこれ、early returnで考えてみますか
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
テンプレートでの使われかたを見るにIsVisibleの判定は必要なさそう
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
どっちにせよ、ここは修正が必要ですね
変数の使用をやめて、MessageにisBotMessageというメソッドを生やしてみました |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slackの僕の発言より引用
要は msg の横に表示すべきアイコンの正しい決定方法がわからんなぁ、と
たぶんAPI叩くと、各発言のアイコンを1個ずつ書き換えられるんですよね。
だから userIconUrl で最優先すべきは msg.Icons なのかなって。また関数名は messageIconURL になる。
- msg.Icons があればそれを使う
- なければ、ボットならmsg.BotIDをIDとして、ボットじゃなければ msg.User を ID として、Userをとってその user.Profile.Image48 を使う
- Userが取れなかったらデフォルトアイコンにフォールバック
このロジックが正しいんじゃないかと思うんですが、確信が持てない。
@kuu ちょっと検討してみてください。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
おっとまちがえた。上記検討してみてください。
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMです! diff で 2か所のアイコンが挿入されたことを確認できました!
botのアイコンが表示されないケースがあったので修正
ついでにGetUserの部分を外に出して変数のokをuserFoundに変更していますが、問題があれば指摘をお願いします。