checkStampを非Vue化した #8194

mousu-a · 2024-11-14T04:48:09Z

Issue

check-stamp.vueを非Vue化したい #8103

概要

提出物のshowページ、日報のshowページのチェックスタンプを非Vue化しました。

変更確認方法

chore/check-stamp-non-vue-conversionをローカルに取り込む
1. git fetch origin chore/check-stamp-non-vue-conversion
2. git checkout chore/check-stamp-non-vue-conversion
komagataでログインする(メンターであれば誰でも良いです)
日報一覧画面にて適当な日報を選び、"日報を確認"ボタンを押して右上にチェックスタンプが表示されることを確認する
"日報の確認を取り消す"ボタンを押してチェックスタンプが消えることを確認する
日報を確認していない状態でコメントをしようとするとアラートが表示されることを確認する
確認した状態では表示されないことを確認する
提出物一覧画面にて適当な提出物を選び、上記手順と同じように確認する。
（提出物では3,4の確認だけで大丈夫です）

Screenshot

提出物showページのチェックスタンプ

* 日報showページのチェックスタンプ

mousu-a · 2024-11-19T00:05:18Z

@Ryooo-k
お疲れ様です！
お手すきの際にこちらレビューをお願い出来ますでしょうか🙏
全く急ぎではないです！

ご都合合わなければ遠慮なくおっしゃってください🙏
よろしくお願いいたします🙇

Ryooo-k · 2024-11-19T01:19:06Z

@mousu-a
レビューさせていただきます👍
終わりましたら改めてご連絡いたします。（１週間以内を目処に）

Ryooo-k

@mousu-a
コメントしました！
ご確認お願いいたします。

Ryooo-k · 2024-11-19T07:11:24Z

app/views/checks/_check-stamp.html.slim

@@ -0,0 +1,6 @@
+.stamp.stamp-approve.show.is-hidden


showとis-hiddenを入れている理由がわからなかったので教えてください🙇
（showとis-hiddenを削除しても正常に動作したので気になりました。）

質問ありがとうございます！！

is-hiddenに関しては、まずはチェックスタンプを非表示にしておかないと画面のロード時に一瞬空のチェックスタンプが表示されてしまうのでつけています！
（デフォルト(HTML読み込み時)ではスタンプは見えない状態に、読み込み後のJSでチェックが確認できれば見える状態にする　という流れにしたい感じです）

すみません、showは自分でも理由がわかりませんでした😅　消しました！

なるほど！教えていただきありがとうございます！勉強になりました✨

Ryooo-k · 2024-11-19T07:36:16Z

app/views/checks/_check-stamp.html.slim

+    | 確認済
+  time.stamp__content.is-created-at
+  .stamp__content.is-user-name
+    .stamp__content-inner


現状のコードから生成されるHTMLは、

<div class="stamp stamp-approve show"> <h2 class="stamp__content is-title">確認済</h2> <time class="stamp__content is-created-at">2024/11/20</time> <div class="stamp__content is-user-name">komagata</div> </div>

となりますが変更前のファイルを見る限り、下記のHTMLを生成するコードが正しいのではないかと思いました。

<div class="stamp stamp-approve show"> <h2 class="stamp__content is-title">確認済</h2> <time class="stamp__content is-created-at">2024/11/20</time> <div class="stamp__content is-user-name"> <div class="stamp__content-inner">komagata</div> </div> </div>

現状だとユーザー名が下よりになっています。

変更すると中央に揃います。

Ryooo-k · 2024-11-20T03:31:49Z

app/javascript/check-stamp.js

+
+      checkStampElement.classList.remove('is-hidden')
+
+      const checkedUserName = document.querySelector('.is-user-name')


こちらが正しいと思いますがいかがでしょうか？👀
（こちらのコメントをご確認ください。）

Suggested change

const checkedUserName = document.querySelector('.is-user-name')

const checkedUserName = document.querySelector('.stamp__content-inner')

ご指摘の通りこれだとHTMLが変わってしまっていました💦
修正しました！

Ryooo-k · 2024-11-20T03:36:57Z

app/javascript/check-stamp.js

+      if (!json[0]) return checkStampElement.classList.add('is-hidden')
+
+      checkStampElement.classList.remove('is-hidden')


toggle()メソッドを使うと１行で書けそうです！

Suggested change

if (!json[0]) return checkStampElement.classList.add('is-hidden')

checkStampElement.classList.remove('is-hidden')

checkStampElement.classList.toggle('is-hidden', !json[0])

第二引数の存在を知りませんでした。。ありがとうございます🙏
toggleでコードを一行に、その後未チェックであればreturnするように変更しました！

mousu-a · 2024-11-20T12:42:13Z

@Ryooo-k
お疲れ様です！
レビューありがとうございます！！
修正しましたので再度ご確認のほどお願いいたします🙇‍♂️

Ryooo-k

@mousu-a
修正ありがとうございます✨
確認しました！OKです🙆

mousu-a · 2024-11-21T03:12:16Z

@Ryooo-k
レビューありがとうございました〜！

@komagata
チームメンバーから確認いただけましたのでこちらレビューをお願いいたします🙇‍♂️

komagata · 2024-11-22T07:51:06Z

app/javascript/checkable.js

@@ -1,3 +1,5 @@
+import setChecks from 'check-stamp.js'


@mousu-a 外部から使われるモジュールとしてわかりやすい設計・ネーミングにした方がいいように思いました。
default exportだとするとcheckStampとかのそのままの名前で使えた方がいいように思いました。
既存の作りはvueで出来てるのは無視して、今わかりやすいように作っていただいて大丈夫です。

すみません、外からわかりやすくという視点が抜けていました😢
名前付きエクスポートにし、setCheckStampで統一しました！
beb140e

mousu-a · 2024-11-25T21:10:47Z

app/javascript/check-stamp.js

@@ -12,6 +12,10 @@ export const setCheckStamp = () => {

  const checkableType = checkStamp.getAttribute('data-checkable-type')
  const checkableId = checkStamp.getAttribute('data-checkable-id')
+  store.dispatch('setCheckable', {


この処理(store.jsのcheckIdのセット)が遅いと、commentsテスト(日報にコメントをする処理のあるテスト)でコメントをする時に、日報をチェック済みかどうか？の確認(store.jsのcheckIdのセット)がコメントをするタイミングに間に合わず、日報がチェック済みであるにも関わらずアラートが出てしまうことがあります。それによりテストが落ちやすくなっていたため、コードを上に移動させました。

9a5cc64

mousu-a force-pushed the chore/check-stamp-non-vue-conversion branch from 221ca41 to 0d010c2 Compare November 18, 2024 02:15

mousu-a self-assigned this Nov 18, 2024

mousu-a requested a review from Ryooo-k November 19, 2024 03:58

mousu-a marked this pull request as ready for review November 19, 2024 03:59

Ryooo-k reviewed Nov 20, 2024

View reviewed changes

Ryooo-k approved these changes Nov 21, 2024

View reviewed changes

mousu-a requested a review from komagata November 21, 2024 03:13

komagata reviewed Nov 22, 2024

View reviewed changes

mousu-a added 8 commits November 25, 2024 22:06

check-stamp.vueをRailsで実装(非Vue化)した

d94b76c

不要なファイル、記述を削除

9a0fd7a

不要な記述だったため削除

8cef7c8

冗長なコードだったため変更

fcbd33f

"未チェック"状態を変数を導入してわかりやすくした

3416639

生成されるHTMLが変わってしまっていたため修正

8a6ffbd

関数名を実態に即した形に

beb140e

comments.rbのテストが落ちやすくなっていたためコードを移動

9a5cc64

mousu-a force-pushed the chore/check-stamp-non-vue-conversion branch from 3bd24a3 to 9a5cc64 Compare November 25, 2024 13:07

mousu-a commented Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checkStampを非Vue化した #8194

checkStampを非Vue化した #8194

mousu-a commented Nov 14, 2024 •

edited

Loading

mousu-a commented Nov 19, 2024

Ryooo-k commented Nov 19, 2024 •

edited

Loading

Ryooo-k left a comment

Ryooo-k Nov 19, 2024

mousu-a Nov 20, 2024

Ryooo-k Nov 21, 2024

Ryooo-k Nov 19, 2024 •

edited

Loading

Ryooo-k Nov 20, 2024 •

edited

Loading

mousu-a Nov 20, 2024

Ryooo-k Nov 20, 2024

mousu-a Nov 20, 2024 •

edited

Loading

mousu-a commented Nov 20, 2024 •

edited

Loading

Ryooo-k left a comment •

edited

Loading

mousu-a commented Nov 21, 2024

komagata Nov 22, 2024 •

edited

Loading

mousu-a Nov 25, 2024 •

edited

Loading

mousu-a Nov 25, 2024 •

edited

Loading


		checkStampElement.classList.remove('is-hidden')

		const checkedUserName = document.querySelector('.is-user-name')

		if (!json[0]) return checkStampElement.classList.add('is-hidden')

		checkStampElement.classList.remove('is-hidden')

checkStampを非Vue化した #8194

Are you sure you want to change the base?

checkStampを非Vue化した #8194

Conversation

mousu-a commented Nov 14, 2024 • edited Loading

Issue

概要

変更確認方法

Screenshot

mousu-a commented Nov 19, 2024

Ryooo-k commented Nov 19, 2024 • edited Loading

Ryooo-k left a comment

Choose a reason for hiding this comment

Ryooo-k Nov 19, 2024

Choose a reason for hiding this comment

mousu-a Nov 20, 2024

Choose a reason for hiding this comment

Ryooo-k Nov 21, 2024

Choose a reason for hiding this comment

Ryooo-k Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Ryooo-k Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a Nov 20, 2024

Choose a reason for hiding this comment

Ryooo-k Nov 20, 2024

Choose a reason for hiding this comment

mousu-a Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a commented Nov 20, 2024 • edited Loading

Ryooo-k left a comment • edited Loading

Choose a reason for hiding this comment

mousu-a commented Nov 21, 2024

komagata Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a commented Nov 14, 2024 •

edited

Loading

Ryooo-k commented Nov 19, 2024 •

edited

Loading

Ryooo-k Nov 19, 2024 •

edited

Loading

Ryooo-k Nov 20, 2024 •

edited

Loading

mousu-a Nov 20, 2024 •

edited

Loading

mousu-a commented Nov 20, 2024 •

edited

Loading

Ryooo-k left a comment •

edited

Loading

komagata Nov 22, 2024 •

edited

Loading

mousu-a Nov 25, 2024 •

edited

Loading

mousu-a Nov 25, 2024 •

edited

Loading