-
Notifications
You must be signed in to change notification settings - Fork 71
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
プラクティス完了ツイート機能の実装 #3385
プラクティス完了ツイート機能の実装 #3385
Conversation
c7492f5
to
9645997
Compare
@machida 「完了Tweetする」ボタンプラクティス詳細ページ: /practices/:id https://github.com/fjordllc/bootcamp/blob/feature/tweet-learning_completion/app/views/practices/show.html.slim#L129-L136 プラクティス完了ツイート用モーダルプラクティス編集ページ/practices/:id/edit https://github.com/fjordllc/bootcamp/blob/feature/tweet-learning_completion/app/views/practices/_form.html.slim#L76-L80 プラクティス完了ページ/practices/:practice_id/completion https://github.com/fjordllc/bootcamp/blob/feature/tweet-learning_completion/app/views/practices/completion/show.html.slim また、テスト用のOGP画像として拾ってきたフリー画像を使っているのですが、余力があれば実際に使いそうなものに差し替えてもらえると助かります |
9645997
to
8189301
Compare
テストが落ちているのはデザイン適用後に直そうと思います |
先にレビュー依頼して、デザインは最後にやってもらうことになりました |
8189301
to
2901dfa
Compare
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.
@haruguchi-yuma
レビューお願いします🙏
label#js-complete.a-button.is-md.is-warning.is-block( | ||
@click='pushComplete', | ||
for='modal-learning_completion' |
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.
プラクティス詳細ページの完了ボタンですが、button要素からlabel要素に変更し、for属性を追加しました
理由としては、完了ボタンを押したときにモーダル表示されるようにするためです
label要素がクリックされると for属性の値 modal-learning_completion
をidとする要素がcheckedになります
モーダルがcheckedになると、sassの設定により画面上に表示されます
bootcamp/app/assets/stylesheets/blocks/shared/_modal.sass
Lines 5 to 6 in 2901dfa
input:checked + & | |
display: block |
@@ -1,6 +1,8 @@ | |||
- title @practice.title | |||
- category = @practice.category(current_user.course) | |||
|
|||
= render '/shared/modal_learning_completion', practice: @practice, tweet_url: @tweet_url, should_display_message_automatically: false |
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.
@@ -0,0 +1,10 @@ | |||
- title @practice.title | |||
- if @practice.ogp_image.attached? | |||
- set_meta_tags(og: { image: polymorphic_url(@practice.ogp_image), url: request.url }) |
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.
OGP画像のパスはホスト名を含む絶対パスでないといけないようなので、そのように設定しました
polymorphic_url(@practice.ogp_image)
で画像の絶対パスが取れるのは以下のブログ記事で見つけた話で、正直根拠がよく分かりません(公式のドキュメントを見てもそういう記載が見つからない)
参考: https://qiita.com/shou8/items/3c41cb6a06ff1e0cdb95#urlが欲しい
記事内で polymorphic_url
と並んで載っている url_for(@practice.ogp_image)
だと相対パスになってしまい、この理由も分かりません
とりあえず上手く動いているので polymorphic_url
を採用してます
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.
https://api.rubyonrails.org/classes/ActionView/RoutingUrlFor.html#method-i-url_for
調べてみたんですが、公式APIにはhostを指定しない場合はデフォルトでonly_path: true
になるみたいでurl_forを使うなら、オプションを使う必要がありそうです。
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.
@haruguchi-yuma
ありがとうございます
なるほど、そういう理由で相対パスになるんですね
確かにhostオプションの指定や only_path: false
で上手く動けばいいんですが、文法的にアウトになっちゃうんですよね(引数は0〜1個しか取れない)
wrong number of arguments (given 2, expected 0..1)
ちょっと url_for
を使うのは難しそうなので、 polymorphic_url
のままにしてみます
module LearningDecorator | ||
def should_display_message_automatically?(current_user:) | ||
user == current_user && complete? && !completion_message_displayed? | ||
end | ||
end |
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.
モーダルの初回表示判定はviewでしか使わないのでメソッドはdecoratorにしましたが、
今見ると権限系に近い処理なので、モデルに直接生やした方がよかったのか迷ってます
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.
現在viewでしか使わず、今後も他で使われなさそうであればこれで良いと思います〜
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.
遅くなってすみません!
twitter周りや、modalの作り方等自分が知らないことばかりでとても勉強になりました。😄
自分がみておかしいと思うところは特になかったのですが1、2点コメントしていますので確認お願いします。
def tweet_url(practice_title:) | ||
completion_text = "フィヨルドブートキャンプのプラクティス「#{practice_title}」が完了しました🎉🎉🎉" | ||
# ref: https://developer.twitter.com/en/docs/twitter-for-websites/tweet-button/guides/web-intent | ||
tweet_param = URI.encode_www_form(text: completion_text, url: request.url, hashtags: 'fjordbootcamp') |
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.
プラクティス完了ツイートには、プラクティスごとに存在するプラクティス完了ページへのリンクURLを記載し、ツイートにはプラクティスのOGP画像が表示されるようにする
ツイートに記載されてるurlはプラクティス完了ページへ遷移するurlだと思うのですが、このままだとプラクティス詳細ページに飛んでしまうので、request.url + '/completion
とした方が良さそうです。
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.
これは気づきませんでした、指摘ありがとうございます🙏
修正前は
- プラクティス詳細ページからツイートしたときはプラクティス詳細ページに飛ぶ
- 提出物詳細ページからツイートしたときは提出物詳細ページに飛ぶ
となっていて、URLに completion
を足すだけだと対処できなさそうなので、
practice_completion_url
でURLを返すようにしました
ついでに tweet_url
内で practice.title
と practice.id
を使うようになったので、引数は practice
に変更しました
0fa7391
module LearningDecorator | ||
def should_display_message_automatically?(current_user:) | ||
user == current_user && complete? && !completion_message_displayed? | ||
end | ||
end |
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.
@haruguchi-yuma
修正したので再度確認お願いします🙏
def tweet_url(practice_title:) | ||
completion_text = "フィヨルドブートキャンプのプラクティス「#{practice_title}」が完了しました🎉🎉🎉" | ||
# ref: https://developer.twitter.com/en/docs/twitter-for-websites/tweet-button/guides/web-intent | ||
tweet_param = URI.encode_www_form(text: completion_text, url: request.url, hashtags: 'fjordbootcamp') |
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.
これは気づきませんでした、指摘ありがとうございます🙏
修正前は
- プラクティス詳細ページからツイートしたときはプラクティス詳細ページに飛ぶ
- 提出物詳細ページからツイートしたときは提出物詳細ページに飛ぶ
となっていて、URLに completion
を足すだけだと対処できなさそうなので、
practice_completion_url
でURLを返すようにしました
ついでに tweet_url
内で practice.title
と practice.id
を使うようになったので、引数は practice
に変更しました
0fa7391
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.
確認しました。これで大丈夫だと思いますー!😃
@komagata |
こちらのIssue、machidaが引きつぎますー |
|
||
private | ||
|
||
def tweet_url(practice:) |
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.
これも Practice#tweet_url
で呼べるようにしたほうがいいですかね…?
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.
基本的に動詞より名詞の方が副作用がない(ように見える)のでプログラムがシンプルになります。
引数なしでいければ、Practice#tweet_url
はクラスのプロパティのように見えるのでそちらの方がわかりやすかな〜と思いました。
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.
すみません、こちら僕がよく理解できていないかもしれません🙇♂️
def tweet_url(practice:)
を def practice_tweet_url
に変更しました。
def Practice#tweet_url
だとrubocopに引っかかるため、上記のメソッド名にしました。
僕の認識がズレていたらご指摘ください🙇♂️
@komagata |
これはなぜでしょう? |
@komagata ここまで書いて思ったんですが、このPRのAssigneeはデザインを担当するmachidaさんが引き継いでます @machida
|
@konaga-k コンフリクトしているのはjsとymlなのでデザイン関係ないので解消できるのではと思うのですがいかがでしょう? |
📝 @komagata のレビュー待ち。 |
c3ce55e
to
122c058
Compare
@komagata |
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.
確認しました、OKですー🙆♂️
@machida コードの方はレビューOKです〜! |
ありがとうございます🙇♂️ |
@komagata ありがとうございます!!では、こちら画像の準備ができるまでコメントアウトしてマージしたいと思います。 |
@machida 了解です〜 |
@komagata コメントアウトしてテストが通ったのでマージしてしまいますー |
refs #2045
概要
モーダルの自動表示制御
プラクティス完了後、提出物詳細ページに初回アクセスしたときのみモーダルを自動表示するため、
learnings
テーブルにモーダル表示済みフラグを追加するUI
「完了Tweetする」ボタン
画像
プラクティス詳細ページ
提出物詳細ページ
画像
プラクティス完了ツイート用モーダル
画像
プラクティス完了ツイート
画像
プラクティス編集ページ
画像
プラクティス完了ページ
画像