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

ブログのWIP機能を実装 #4095

Merged
merged 10 commits into from
Feb 9, 2022
Merged

ブログのWIP機能を実装 #4095

merged 10 commits into from
Feb 9, 2022

Conversation

kaisumi
Copy link
Contributor

@kaisumi kaisumi commented Jan 31, 2022

Issue: #3730

  • Articlesテーブルにwipとpublished_atを追加
  • WIPボタンを表示(new、edit)
  • wip: false && published_at: nilのデータについてpublished_at = created_atとするデータマイグレーションを追加
  • indexとshowでpublished_atまたは'執筆中'+WIPアイコンを表示

@kaisumi
Copy link
Contributor Author

kaisumi commented Jan 31, 2022

@machida
お疲れ様です!本件、WIPマークとnoticeのデザインをよろしくお願いします。
編集画面の黄色の「更新する」のボタン、今回WIPが入ったことで「更新して公開する」などに変更した方が良いかと思いました。ボタン名称の変更は町田さんにお願いしていいものでしょうか?

@machida
Copy link
Member

machida commented Jan 31, 2022

@kaisumi はい!僕の方で対応しますー

@machida
Copy link
Member

machida commented Jan 31, 2022

@kaisumi すいません、articlesのseedデータ(fixturesにあるやつ)に投稿日が入ってないので、投稿日を入れるようにしておいてくださいー。あと、WIP状態のデータもそこに追加しておいていただきたいです🙏

@machida
Copy link
Member

machida commented Jan 31, 2022

@kaisumi デザイン入れましたー mainブランチの最新を取り込んだので、このブランチにpushする際は、

git checkout feature/wip-on-blog
git pull --rebase origin feature/wip-on-blog

をお願いします🙏

@machida machida removed their assignment Jan 31, 2022
@kaisumi
Copy link
Contributor Author

kaisumi commented Jan 31, 2022

@machida デザインとコメントありがとうございました!
投稿日はdbの方ですよね?失礼しました。追加します 🙇

@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 1, 2022

@maeda-seina
お疲れ様です!こちらお手隙の時にレビューお願いします 🙇
Lintがコケていますが、以下のPRで対応されるようです:
#4103

Copy link
Contributor

@maeda-seina maeda-seina left a comment

Choose a reason for hiding this comment

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

@kaisumi さん
お疲れ様です。レビュー依頼ありがとうございます。(レビュー遅くなって申し訳ありません!!🙇‍♂️)
動作確認させていただいたところ、LGTMでした!👍(こちらのkomagataさんのコメントを参考にして動作確認させて頂きました!)
WIP機能の実装やdata migrationの部分などとても勉強になりました〜!😄

# frozen_string_literal: true

class CopyTimestampToPublishedAtForBlog < ActiveRecord::Migration[6.1]
def up
Copy link
Contributor

Choose a reason for hiding this comment

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

既存のデータ(published_atカラムの中身がnilのデータ)のままではエラーが出てしまう箇所があるんじゃないかな?と思っていたのですが、このような形でdata migrationを書けば良いのですね!!
個人的にとても参考になりました!!

@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 4, 2022

@maeda-seina お疲れ様です!レビューありがとうございました 😄

@komagata お疲れ様です!お手すきの時にレビューをお願いいたします 🙇

@kaisumi kaisumi requested a review from komagata February 4, 2022 05:28
Comment on lines 9 to 13
if admin_or_mentor_login?
Article.all.order(created_at: :desc)
else
Article.all.where(wip: false).order(created_at: :desc)
end
Copy link
Member

Choose a reason for hiding this comment

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

ここをメソッド化して直感的な名前をつけたら読みやすくなるかもと思いまし〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata コメントありがとうございます 😄 変更してみました。
ご確認をよろしくお願いします 🙇

@@ -8,6 +8,8 @@ html.is-application lang='ja'
= javascript_include_tag 'application'
= javascript_pack_tag 'application'
= csrf_meta_tags
- if defined?(@article) && @article.wip?
meta name="robots" content="none"
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

確認しました、OKですー🙆‍♂️

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.

4 participants