ダッシュボードのブックマーク一覧をreactにした #7161
Conversation
|
@junohm410 |
|
@ham-cap |
|
@junohm410 |
There was a problem hiding this comment.
@ham-cap
遅くなりすみませんでした🙇♂️確認させていただきました。
FileChanged以外の部分で、気になった点を下にシェアさせていただきます。
受講生のダッシュボードで余白がない
私の手元では、下のようにカード間の余白がなくなっていましたので、念の為ご確認いただけるとありがたいです🙇♂️
（下はkimuraでログインしています。なお、アドバイザー・メンターでは問題なく余白がありました）
ブックマークを1件の状態から削除して0件にすると、空のカードが出る
現状mainの仕様では1件から削除して0件にすると（リダイレクトされて）ブックマークのカードそのものが非表示になりますが、本PRの変更内容だと空のカードが表示されます。
対応策を考えてみたのですが、たとえば下記のようにブックマークが0件の時は関数コンポーネントはnullを返してカード自体を非表示にするなどの方法があるかなと感じました。
if (data.unpagedBookmarks.length === 0) {
return null
}非表示にした際の余白を持たせられるかどうかは、一つ前にシェアした余白の問題と絡みそうな気がするので、上の対応策のまま行けるかは自信がないのですが、ご確認いただけるとありがたいです🙏
current_user/bookmarks_controllerのdestroyアクションは消せそう？
# current_user/bookmarks_controller
def destroy
Bookmark.find(params[:id]).destroy
redirect_to root_path, notice: 'Bookmarkを削除しました。'
end今回の変更でダッシュボードからのブックマークの削除はAPIを叩いて行うようになったので、slim版で削除のために使っていた上記のアクションとルーティングは削除できるかもしれないと考えました。
destroyアクションは、slim版のダッシュボードのブックマーク削除機能の追加時に、それ用に追加されたもののようです↓
ただ、他の場所で使っている可能性を考えると触るのは怖い気もするので、別Issueで調査の上消すなどの進め方もあるのかな？とも考えたのですが、こちらも念の為ご確認いただけると幸いです🙏
（すでに他の場所で使われていることを確認されているのであれば、大変申し訳ないです🙇♂️）
|
@junohm410 |
There was a problem hiding this comment.
お疲れ様です！ご対応・ご連絡ありがとうございました🙏
0件のときにカードを表示しないようにする件、ジャストアイデアで関数コンポーネントでnullを返す方法を書きましたが、日報や分報を拝読するとそんな簡単な問題ではなかったようで、お恥ずかしい限りです🙇♂️
今回はreact_componentのヘルパーを使わないことにされていますが、それについても任意のクラス名をつける方法など知らないことばかりで、コミットの流れから大変勉強させていただきました🙏ありがとうございます！
FileChangedで気になった点（ほぼ質問です）をレビューで残しました。
また別件で、コンフリクトが起きているようです。
念の為サーバを立ち上げて動きを見ようとしたところ、以下のように出て起動することができませんでした。
You have already activated stringio 3.1.0, but your Gemfile requires stringio 3.0.1. Prepending `bundle exec` to your command may solve this. (Gem::LoadError)
stringioのアップデート（7189での変更）など最新のorigin/mainの変更が取り込まれていないと思うので、こちらのご対応の必要がありそうかなと思いました。
以上、ご確認いただけますと幸いです🙏よろしくお願いいたします。
ham-cap
force-pushed
the
feature/change-bookmark-list-in-dashboard-to-react
branch
2 times, most recently
from
678a908
to
6d9cd2f
Compare
|
@junohm410
こちら、ご指摘いただいておりました点について重複をなくして書いてみましたのでご確認ください🙏 よろしくお願いいたします🙇♂️ |
| ) | ||
| } | ||
|
|
||
| const Bookmark = ({ bookmark, editable, bookmarksUrl, _setEditable }) => { |
There was a problem hiding this comment.
_setEditable はこのコンポーネントでは使っていない感じですよね？👀であれば、そもそもpropsとして渡さないようにすればよりわかりやすいのではないかなと考えました（気づいていない理由があれば申し訳ないです🙏）。
|
@junohm410 @komagata |
| import React from 'react' | ||
|
|
||
| export default function AdminCompanies({ | ||
| companyId, | ||
| dataTitle, | ||
| dataMentorLogin, | ||
| dataCurrentUserId | ||
| }) { | ||
| console.log(dataMentorLogin) | ||
| return ( | ||
| <div> | ||
| <p>カンパニーID: {companyId}</p> | ||
| <p>データタイトル: {dataTitle}</p> | ||
| <p>メンターログイン: {dataMentorLogin}</p> | ||
| <p>カレントユーザーID: {dataCurrentUserId}</p> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
すみません、komagataさんにご連絡後に申し訳ないのですが、こちらに不要に変更が含まれてないでしょうか？🙏
| const Bookmark = ({ bookmark, editable, bookmarksUrl, _setEditable }) => { | ||
| const Bookmark = ({ bookmark, editable, bookmarksUrl }) => { |
There was a problem hiding this comment.
38行目でsetEditableを渡しているところも削除できると思います👀
ham-cap
force-pushed
the
feature/change-bookmark-list-in-dashboard-to-react
branch
2 times, most recently
from
f7d7d25
to
63f035e
Compare
|
@junohm410 |
|
@junohm410 @komagata |
| @@ -17,8 +17,7 @@ | |||
| .a-panels__item | |||
| = render 'job_seeking_users', users: @job_seeking_users | |||
| - if current_user.bookmarks.present? | |||
| .a-panels__item | |||
| = render 'bookmarks', user: current_user | |||
| div id='bookmarks-in-dashboard' | |||
There was a problem hiding this comment.
ほかの部分と同じようにreact_componentヘルパーを使うのがいいと思います～
There was a problem hiding this comment.
react_componentを使う場合に生じる問題として、ブックマークの数が0になってブックマーク一覧そのものを非表示にしたくてもコンポーネント自体のdiv要素が残ってしまい、次にページ全体が更新されるまで余計な空白が残ってしまうというものがあったため現在の実装にしているのですがまずいでしょうか？💦
react_componentヘルパーを使用する場合でもこの問題をクリアできる方法にお心当たりがありますでしょうか？👀
There was a problem hiding this comment.
@komagata
ご確認いただきありがとうございます🙏
こちらでも手元でもう一度確認してみましたが、やはりコンポーネントのdivは残ってしまっているようです👀
コミットしていただいたアドバイザーのダッシュボードの場合は各項目が縦に並んでいるため分かりにくいのですが、例えばブックマークとWIPを一つずつ登録して縦に並んだ状態からブックマークを全て削除した場合、一見divごと消えているように見えてしまいますが、ブックマークの一覧がそもそも存在しない場合と比べると微妙に不必要なスペースが空いてしまいます。（以下のスクショをご参照ください。ページ全体をリロードするとdivが無くなるため不必要なスペースは解消します。）
デベロッパーツールでも確認しましたが、ダッシュボードのページが再読み込みされるまではやはりコンポーネントのdiv（data-react-class="BookmarksInDashboard"）が残っております。
こちらの状態から、
ブックマークを全て削除すると以下のようになります。
以下が通常の場合です。
なお、メンターのダッシュボードの場合ですと、各項目が横並びのため不必要なスペースが顕著に見られます。
また、受講生のダッシュボードでは、アドバイザーと同様に項目が縦並びのため分かりにくいですが、やはりreact_componentでコンポーネントの呼び出しを行うとdivが残り、レイアウトが崩れます。
受講生はHTMLの階層構造が異なるためSassのあたり方が異なり、項目間のスペースが極端に狭くなります。
当初は私も公式のドキュメントを参照しながらreact_cpmponentを使用しての実装を試みたのですが、このヘルパーはコンポーネントをマウントするためのdivでラップしてしまうため、コンポーネントのアンマウントをしても一番外側のdivが残ってしまう問題を解決できず、現状の実装にしております。
私が解決策を見逃してしまっているだけの可能性もありますので、他に良さそうな手段があればご教示いただければ嬉しいです🙏
There was a problem hiding this comment.
@ham-cap う～ん、やっぱり僕の環境（新しいPCでも確認）だとDIVが残らないみたいです。
ユーザーはkomagataとhajimeで確認しました。
もうしわけないのですが、他の生徒の方にも確認してもらってもよいでしょうか。
There was a problem hiding this comment.
@komagata
なるほどです👀
承知いたしました！
@junohm410
お疲れ様です！
先日レビューしていただいたこちらのPRにつきまして、何度もお手数をおかけしてしまい申し訳ないのですが、検証にご協力いただけないでしょうか。
レビューの際と同様にこちらのブランチを取り込んでいただき、app/views/home/_adviser_dashboard.html.slimの20行目、コンポーネントを読み込んでいる部分を= react_component 'BookmarksInDashboard'にしたうえで、ブックマークを0件にしたときにコンポーネントのdiv要素が残って余計な余白が表示されていないかご確認いただけると大変助かります🙇♂️
ご不明な点がありましたらお知らせください。
よろしくお願いいたします🙏
There was a problem hiding this comment.
@ham-cap
お疲れ様です！確認しました。
上でのやり取りである通り、WIPのカードがある方が見た目的にも検証しやすそうだったので、下記の条件で検証しました。
- Chromeのバージョン: 123.0.6312.59（Official Build） （arm64）
- 本ブランチを取り込み、
advijirouでログイン - WIPのドキュメントを作成（ダッシュボードにWIPのカードが表示されるようにする）
- ブックマークを1件新規追加
- ダッシュボードのブックマークカードの編集機能を使い、ブックマークを0件にする
- 更新しない状態で要素を確認
私の手元では、data-react-class='BookmarksInDashboard'を持つdivが残っていました😥
ご確認のほど、よろしくお願いいたします。
There was a problem hiding this comment.
@junohm410
早速ご確認いただきありがとうございます！
お手数をおかけいたしました🙏
大変助かりました🙏
@komagata
こちら、@junohm410 さんにも確認していただきましたが、やはりdivは残るようです💦
いただいたコメントを読み返してみて一点気になったのですが、「ユーザーはkomagataとhajime」でご確認いただいたとのことだったのですが、advijirouでもお試しいただいておりますでしょうか？👀
というのも、いま触っているのはapp/views/home/_adviser_dashboard.html.slimなので、アドバイザーのダッシュボードでの表示になります。komagataとhajimeはアドバイザーではないと思いますので、app/views/home/_mentor_dashboard.html.slimかapp/views/home/index.html.slimが表示されることになります。
この二つに関しては現状、私が実装したとおりreact_componentヘルパーを使用しないパターンになっており、divが消えるようになっておりますので、komagataとhajimeでログインしてもapp/views/home/_adviser_dashboard.html.slimの検証にはなっていないような気がします👀
この点、念のためご確認いただければと思います🙏（advijirouでも確認した上でのコメントということであれば申し訳ありません🙇♂️）
There was a problem hiding this comment.
@ham-cap advijirouでやってみたらdivが残っていました～。
なるほどですね。僕が変えたのはアドバイザー用のページだけだったんですね。
ただ、ここだけ特別にreact_componentを`使わないのは良くない（というか方法がありそうに思う）のでもう少し実装方法を調べてみます。
There was a problem hiding this comment.
@ham-cap ( cc @komagata @junohm410 )
div は残したままで大丈夫です。
残った div に、hidden という class を付与できますでしょうか？
可能でしたら、ブックマークが0になったら、残った中身のない div に hidden という class を付けていただきたいです。
hidden を付けると display: none になるので、そこに div があっても余計なマージンは発生しなくなります。
komagata
force-pushed
the
feature/change-bookmark-list-in-dashboard-to-react
branch
from
84342e0
to
63f035e
Compare
・BooksInDashboardを誤って二箇所でマウントしてしまっていることに気づかずにhome_testのほうを変更してしまっていたため修正した。
・コンポーネントの呼び出しをreact_componentヘルパーを使用した方法に変更したことに伴い不要となったため削除した。
ham-cap
force-pushed
the
feature/change-bookmark-list-in-dashboard-to-react
branch
from
63f035e
to
46acf15
Compare
|
@machida @komagata @junohm410 |
|
@ham-cap |
|
@junohm410 |
|
@komagata |
|
@ham-cap メンターはOKでしたー👍 |
|
@ham-cap アドバイザーもOKでした！！🙆 |
|
@machida |
Issue
概要
ダッシュボードにあるブックマークの一覧はviewのテンプレートで実装されていましたが、それらをReactコンポーネントに置換しました。
変更確認方法
feature/change-bookmark-list-in-dashboard-to-reactをローカルに取り込む編集ボタン
削除ボタン
削除後
Screenshot
今回のissueは既存の表示をRailsのViewファイルからReactへ置換するもののため、変更後も見た目上の変化はほとんどありませんが、一点だけ、ブックマークを削除した際に、これまでは「ブックマークを削除しました。」の表示が画面上部にフラッシュで表示されていましたが、React化に伴いトースト表示に変更していますのでご確認ください
変更前
変更後