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

report_template.vue, report_template_modal.vueをreactに対応させる #5139

Closed
komagata opened this issue Jun 30, 2022 · 19 comments
Closed
Assignees

Comments

@komagata
Copy link
Member

komagata commented Jun 30, 2022

下記を参考にしてreactに変える。

@github-actions
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@github-actions github-actions bot added the stale label Aug 30, 2022
@komagata komagata removed the stale label Aug 30, 2022
@komagata komagata changed the title report_template.vue, report_template_modal.vueをVueMounterに対応させる report_template.vue, report_template_modal.vueをreactに対応させる Sep 17, 2022
@github-actions
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@github-actions github-actions bot added the stale label Nov 17, 2022
@komagata komagata removed the stale label Nov 18, 2022
@github-actions
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@github-actions github-actions bot added the stale label Jan 18, 2023
@komagata komagata removed the stale label Jan 23, 2023
@github-actions
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@github-actions github-actions bot added the stale label Mar 25, 2023
@komagata komagata removed the stale label Mar 28, 2023
@github-actions
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@github-actions github-actions bot added the stale label May 28, 2023
@komagata komagata removed the stale label May 29, 2023
@github-actions
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@github-actions github-actions bot added the stale label Jul 29, 2023
@komagata komagata removed the stale label Jul 29, 2023
@github-actions
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@github-actions github-actions bot added the stale label Sep 28, 2023
@komagata komagata removed the stale label Sep 29, 2023
Copy link

このissue|PRは60日間更新がないため7日後にcloseします。closeしたくない場合はstaleラベルを外してください。

@junohm410
Copy link
Contributor

junohm410 commented Dec 21, 2023

@komagata
CC: @machida

お疲れ様です。
日報テンプレート機能のコンポーネントをReact化する件で、一点質問したいことがありご連絡します。

聞きたいこと

現状の「編集を保存せずにモーダルを閉じて、もう一度編集モーダルを開くと、先ほど編集をしていた文面がセットされている」という挙動は、維持したほうがいいでしょうか?

挙動のスクショ

liB80MvjVt

現状の仕様

現状のVueでは、下記のような処理で上記の動きを行っています。

  • 編集中のテンプレートの入力内容をreport_template.vue(テンプレート登録・反映のボタンのコンポーネント)のeditingTemplateプロパティで保管。
  • それを子の編集画面のモーダルreport_template_modal.vueに渡す。

子のモーダルを閉じるcloseModalイベントハンドラの処理で、editingTemplateをモーダルを閉じる直前の編集状態に更新することで、次にモーダルを開けたときに、前回の編集状態から編集を再開することができるようになっています。

考えたこと

現在これをReactで書き換えていますが、これを実現するには、親のコンポーネントでeditingTemplatestateを持ち、それを子のモーダルに渡す必要があると考えています。

// ReportTemlateコンポーネント

export default function ReportTemplate({ templateDescription, templateId }) {
  // ...
  const [editingTemplate, setEditingTemplate] = useState(registeredTemplate)

// ...

{showModal && (
  <ReportTemplateModal
    // ...
    editingTemplate={editingTemplate}
    setEditingTemplate={setEditingTemplate}
    closeModal={closeModal}
  />
)}
// ReportTemplateModalコンポーネント

<textarea
  // ...
  value={editingTemplate} // 初期値にstateで保存している最新のテンプレートの下書きを設定
  onChange={(e) => setEditingTemplate(e.target.value)}></textarea>

いったんこのような形で作業を進めました。
#7164

しかしこの場合、子のモーダルでテンプレートの編集の入力をするたびに、stateを管理する親のボタンのコンポーネントまで再レンダリングされるので、パフォーマンス的に微妙なのではないか、と考えました。

また、あくまで個人の感覚ではあるのですが、そもそもこのモーダルを開いた時は、前回にモーダルを開いていたときの編集内容ではなく、編集のベースとなる「そのときに登録されているテンプレート」が常にセットされる方が自然なのではないかとも考えました。

#3391
#3419
この機能が追加されたときのIssue/PRも見ましたが、とくにこの挙動が指定されて実装された感じではなさそうでした。

考えてみた提案内容

  • 編集中のテンプレートの入力内容は、子のモーダル内で管理する。親と切り離すことで不要な再レンダリングを起こさないようにする。
  • 親での編集中のテンプレートの入力内容の管理をやめるので、モーダルを開くときは常に「登録されているテンプレート」がセットされる。
  • 編集中に誤ってモーダルの外をクリックしてしまい、意図せず編集内容が失われることを防ぐために、「テンプレートの編集内容が今の登録されているものと異なるとき && モーダルの外をクリックして閉じるとき」だけ、「編集モーダルを閉じますか?」的なconfirmを入れる。

上記の動作イメージが下です。

1h8gq5xAv9

編集内容はモーダルを閉じても維持すべきかどうか以外に、

  • そもそもこの程度の再レンダリングはそこまで気にしなくても良い
  • するにしても、書き換えのIssueですべきことではない(別のIssueにわける)
  • 親で編集内容のstateを持ち、渡された子でそのstateをセットしつつ、親側でレンダリングを回避するための手法について調べるべき方向性(refで頑張って書けるかなと思ったのですが、子側では編集内容によって登録ボタンをdisabled=trueするかどうかなどを動的に操作したいので、いずれにしてもeditingTemplateはリアクティブに管理はしたいイメージです)

以上はあくまで例ですが、このようなアドバイスがもしあればご教示いただけるとありがたいです🙏
お手数をおかけしますが、よろしくお願いいたします。

@komagata
Copy link
Member Author

@junohm410

現状の「編集を保存せずにモーダルを閉じて、もう一度編集モーダルを開くと、先ほど編集をしていた文面がセットされている」という挙動は、維持したほうがいいでしょうか?

維持しなくて良いです。

そもそもこの程度の再レンダリングはそこまで気にしなくても良い

気にしなくて良いです。

@junohm410
Copy link
Contributor

@komagata
ご確認いただき、ありがとうございました🙏

それでは、「編集を保存せずにモーダルを閉じて、もう一度編集モーダルを開くと、先ほど編集をしていた文面がセットされている」という挙動は維持せず、

  • 編集中のテンプレートの入力内容は、子のモーダル内で管理する。親と切り離すことで不要な再レンダリングを起こさないようにする。
  • 親での編集中のテンプレートの入力内容の管理をやめるので、モーダルを開くときは常に「登録されているテンプレート」がセットされる。
  • 編集中に誤ってモーダルの外をクリックしてしまい、意図せず編集内容が失われることを防ぐために、「テンプレートの編集内容が今の登録されているものと異なるとき && モーダルの外をクリックして閉じるとき」だけ、「編集モーダルを閉じますか?」的なconfirmを入れる。

こちらで提案させていただいた内容でまずは実装し、メンバー・komagataさんにレビューいただくように進めさせていただきます。

また再レンダリングを気にする温度感的なものもシェアいただきありがとうございます。こちらは今後の参考にさせていただきます!
引き続きよろしくお願いいたします。

@junohm410
Copy link
Contributor

junohm410 commented Dec 27, 2023

@komagata @machida
お疲れ様です。2023/12/27の開発MTG・質問タイムにて、上で提案させていただいた内容に加え、本Issueで下記の変更を含めることを確認させていただきました。
Issue上でも下のように記録しておきます。
(本来は事前にIssue上で確認すべき内容でした。以後、気をつけます。)

編集内容が登録内容と同じ場合、「変更」ボタンが押せないように変更

変更前は、既存のテンプレートから無編集の状態でも「変更」ボタンを押してAPIを叩けていましたが、このシナリオでボタンを押せないように修正する予定です。

  • イメージ
    ZPlbX98w79

「新規登録 -> ページを更新せず再度をモーダルを開き、再編集してテンプレート変更する」というシナリオで、/api/report_templates/nullでAPIを叩いてしまうバグを修正

テンプレートの新規登録時は、コンポーネントにpropsとして渡されるテンプレートのidnullになります。
また現行の実装では、テンプレートの新規登録時に登録後のテンプレートのidをレスポンスとして受け取っていません。
そのため、現行の実装において、「新規登録 -> ページを更新せず再度をモーダルを開き、再編集してテンプレート変更する」というシナリオで、テンプレート更新時に/api/report_templates/nullとしてAPIを叩いてしまっていました。

よって、新規登録時に、登録されたテンプレートのidをレスポンスとして受け取り、それをstateに保管する実装に修正する予定です。
また、そのようなレスポンスを返せるよう、api/report_templates_controllercreateアクションに修正を加える予定です。

コードも含めた説明は下のPRのコメントにあります。
https://github.com/fjordllc/bootcamp/pull/7164/files#r1436196343

@machida
Copy link
Member

machida commented Dec 27, 2023

@junohm410 共有ありがとうございますー🙏

@junohm410
Copy link
Contributor

@komagata
CC: @machida
お疲れ様です。
本件、React化にあたり、もう一件既存のロジックによる問題点を解消できる点がありましたので、ご確認いただきたくご連絡しました。
お手数をおかけしますが、よろしくお願いします。

React化に際して、処理で変更できる点

現状

編集中のテンプレートのマークダウンプレビューは、textareaMarkdowngemとMarkdownInitializer#renderによる2つの処理で行われている。

変更できる点

編集中のテンプレートのマークダウンプレビューは、textareaMarkdowngemによるプレビュー機能のみで記述可能。

確認いただきたい点

上の「変更できる点」で進めて問題ないか、ご判断いただきたいです。
(これまでに相談した内容と同じく、今回は下記のようにすでに進めてしまっておりました🙇‍♂️別Issueとして取り組むべきであれば、元に戻すつもりです)
https://github.com/fjordllc/bootcamp/pull/7164/files#r1436204117

変更できる理由の説明

現行のVueでは、編集中のテンプレートのマークダウンプレビューは、下記のようにtextareaMarkdowngemとMarkdownInitializer#renderによる2つの処理で記述されています。

// app/javascript/report_template_modal.vue
// 中略
    .a-markdown-input.js-markdown-parent
      .a-markdown-input__inner.js-tabs__content(
        v-bind:class='{ "is-active": isActive("template") }')
        textarea.a-text-input.a-markdown-input__textare.has-max-height(
          :id='`js-template-content`',
          :data-preview='`#js-template-preview`', // textareaMarkdownによるプレビュー実装
          v-model='editingTemplate',
          name='report_template[description]')
      .a-markdown-input__inner.js-tabs__content.a-long-text.is-md.a-markdown-input__preview(
        v-bind:class='{ "is-active": isActive("preview") }',
        v-html='markdownDescription') // MarkdownInitializer#renderによるプレビュー実装
        #js-template-preview // textareaMarkdownによるプレビュー実装

// 中略

    markdownDescription() {
      const markdownInitializer = new MarkdownInitializer()
      return markdownInitializer.render(this.editingTemplate) // MarkdownInitializer#renderによるプレビュー実装
    }

現状のVueにマークダウンプレビューの処理が2つある理由

↓こちらのコミットで、マークダウンプレビューがモーダルを開いたタイミングから反映されるようにするために、MarkdownInitializerによる処理が追加されたようでした。
b060d85

問題点

しかし、MarkdownInitializer#renderで得られたhtmlをv-htmlで当該divタグの子要素に入れた結果、同じくそのタグの子要素として記述されたdiv id="js-template-preview"がDOMに反映されず、textareaMarkdownの処理は無意味になっています。
プレビューを表示する、という点では問題ありませんが、不必要な処理がそのままになっていることに違和感を感じます。

React化でできること

Reactでは最初の描画時にpropsのeditingTemplatetextareaの初期値として渡し、そのあとにuseEffecttextareaMarkdownでプレビュー表示の処理を行うことができるので、上記のコミットのようなケアは不要です。

(Vueの場合、textareaの初期値にpropsではなくデフォルト値の''が最初の描画時に使われ、その状態でtextareaMarkdownによるプレビュー処理が行われるため、モーダル起動時のプレビューには空文字が反映されてしまいます。その問題を解決するのが上記コミット。)

また、他の箇所のマークダウンの表示の実装を見てみると、

  • MarkdownInitializer#render -> すでにサブミットされたマークダウンによる投稿などの表示
  • textareaMarkdown -> テキストエリアに記入中のマークダウンのプレビュー表示

というように使い分けられているようだったので、React化にあたりtextareaMarkdown のみでプレビュー機能を実装し、MarkdownInitializerの方は削除する方針がいいのではと思っております。

@komagata
Copy link
Member Author

komagata commented Dec 31, 2023

@junohm410 回答の前に一点質問させてください。(この前提が違うとその後の議論が無意味になるため)

textareaMarkdown gemというのはnpmの間違いですかね?

@junohm410
Copy link
Contributor

@komagata
はい、npmの間違いです。こちらは失礼いたしました🙇‍♂️

@komagata
Copy link
Member Author

@junohm410 なるほどです〜。

:@komagata:でユーザーのアイコンが出るなど、FBC独自の機能も使えているのであればおっしゃっている方法で問題ないです〜。

@junohm410
Copy link
Contributor

@komagata
ご回答ありがとうございました🙏

FBC独自の機能も使えているのであればおっしゃっている方法で問題ないです〜。

この方法でもFBCのMD独自機能は使えると判断しています。
このtextareaを設定する際に、素のtextareaMarkdownを自分で使うのではなく、そのラッパーのTextareaInitializer.initializeというクラスメソッドを使っています。

// app/javascript/textarea-initializer.js

export default class {
  static initialize(selector) {
    const textareas = document.querySelectorAll(selector)
    if (!textareas.length) return

// 中略

    // markdown
    Array.from(textareas).forEach((textarea) => {
      /* eslint-disable no-new */
      new TextareaMarkdown(textarea, {
        endPoint: '/api/image.json',
        paramName: 'file',
        responseKey: 'url',
        csrfToken: CSRF.getToken(),
        placeholder: '%filenameをアップロード中...',
        afterPreview: () => {
          autosize.update(textarea)

          const event = new Event('input', {
            bubbles: true,
            cancelable: true
          })
          textarea.dispatchEvent(event)
        },
        plugins: [
          MarkdownItEmoji,
          MarkdownItMention,
          MarkdownItUserIcon,
          MarkdownItTaskLists,
          MarkDownItContainerMessage,
          MarkDownItContainerDetails,
          MarkDownItLinkAttributes,
          MarkDownItContainerSpeak
        ],
        markdownOptions: MarkdownOption
      })
      /* eslint-enable no-new */
    })

    // user-icon
    new UserIconRenderer().render(selector)

    // Convert selected text to markdown link on URL paste
    new TextareaMarkdownLinkify().linkify(selector)
  }

ここで独自機能用のプラグインやユーザーアイコンの設定を行なっているため、今回の日報テンプレートの入力・プレビューにおいてもFBC独自機能が使えます。
実際に、Docs: フィヨルドブートキャンプで採用しているMarkdownの方言 | FBCにあるものが、この方法での開発環境で全て使えることも確認しました。

image

なので、いったんこの内容で進めさせていただければと思います。
引き続き、何卒よろしくお願いいたします🙏

@junohm410
Copy link
Contributor

本番環境での動作確認ができましたので、本IssueはCloseさせていただきます。

@komagata komagata moved this to 完成 in bootcamp Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants