-
Notifications
You must be signed in to change notification settings - Fork 72
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
bookmark-button.vueをreact対応にする #5114
Comments
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。 |
@komagata 質問1 :
vueではなくなっているので、こちらはなにも変更を加えなくて構いませんか? 質問2:
ただ、これは以下のIssueで現在 @junohm410 さんが取り組んでいるものと変更範囲が被るかもしれません。 こちらはどこまで変更を行えば良いでしょうか。 |
@kyokucho1989 お疲れ様です。
はい、被りそうです。まずどのように被りそうかについて、今日中にここでシェアできるようにします。 すみませんが、よろしくお願いします。 |
変更の概要#7356 のPR(現時点ではドラフト)で、 同部分のブックマークボタンに関わる箇所は、下のRailsのviewファイルに移動させます。 移動先では、現在では下のような形で
このマウント方法は、このQuestionのビューでの独自の手法ではなく、すでに他で
考えられる進め方についてReact化のIssueなので、Questionに限らずですが、slimテンプレート上では下記のような実装に変わるのではないかと思っています。 # app/views/questions/_question_header.html.slim
= react_component('BookmarkButton', bookmarkable-id="#{question.id}", bookmarkable-type='Question') 私のPRは今日明日にはレビューに回せそうなので、kyokuchoさんがReact化を進めているうちにレビューが終わり、私のPRのマージ後に変更を
のようにしてもらえると、不要なコンフリクトを減らせるのではないかなと考えました。 もちろん、React化にあたりpropsを変える必要が出てきそうとか、私のレビューが長くかかったとか、そのような際には都度相談させていただければと存じますので、やりとりを続けさせてもらえればというイメージです🙏 不明点あればDiscordで通話も大丈夫です。 |
@junohm410 |
@kyokucho1989
上のコメントでシェアした実装(既存のDocsなどと同じ形)でVueコンポーネントをマウントしています。 |
本番環境で動作確認できましたのでクローズします。 |
下記を参考にしてreactに変える。
The text was updated successfully, but these errors were encountered: