-
Notifications
You must be signed in to change notification settings - Fork 235
fix: Cannot update existing table data with handson table modal #10551
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
fix: Cannot update existing table data with handson table modal #10551
Conversation
|
|
||
| if (!isOpened) { | ||
| return <></>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dynamic import 後の HandsontableModalSubstance の不要な rerender 対策
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今のままだとモーダルの fade エフェクトがなくなったりするという微妙なデメリットがある
ちゃんとやるなら他のモーダル (ShortcutsModal など) を参考に、一番外側の <Modal> は Container component (lightweight, always rendered) に含むように改修することになる
HandsontableModalSubstance は view / editor の場合分けなしに両対応するとして、
それをラップする HandsontableModal 側で場合分けすべきかなと思う
ちなみに DrawioModal も同じかもしれない
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ShortcutsModal を参考に修正しました。
View / Editor の場合分けは HandsontableModal で行い HandsontableModalSubstance では意識しなくて良いようにしました
|
|
||
| export const HandsontableModalSubstance = (): JSX.Element => { | ||
|
|
||
| const HandsontableModalSubstance = (): JSX.Element => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不要な export の削除
|
|
||
| // Memoize modal open handler | ||
| const handleModalOpen = useCallback(() => { | ||
| const markdownTableState = table == null && editor != null ? getMarkdownTable(editor) : table; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
バグの原因
エディターからモーダルを開く際に、関係のない "table" という変数を見ていたから。"table" は2回目にモーダルを開く時に必ず not null になる。
変数 table について
"table" は useHandsontableModalStatus 由来のもの
growi/apps/app/src/client/components/PageEditor/HandsontableModal/HandsontableModal.tsx
Lines 38 to 40 in 05bc1d5
| const handsontableModalData = useHandsontableModalStatus(); | |
| const isOpened = handsontableModalData.isOpened; | |
| const table = handsontableModalData?.table; |
modal を閉じる際に呼ばれる close メソッドで closeHandsontableModal() が呼ばれる
growi/apps/app/src/client/components/PageEditor/HandsontableModal/HandsontableModal.tsx
Lines 166 to 171 in 05bc1d5
| const cancel = () => { | |
| closeHandsontableModal(); | |
| closeHandsontableModalForEditor(); | |
| setIsDataImportAreaExpanded(false); | |
| setIsWindowExpanded(false); | |
| }; |
closeHandsontableModal() によって useHandsontableModalStatus の table が defaultMarkdownTable になる
growi/apps/app/src/states/ui/modal/handsontable.ts
Lines 16 to 67 in 05bc1d5
| const defaultMarkdownTable = () => { | |
| return new MarkdownTable( | |
| [ | |
| ['col1', 'col2', 'col3'], | |
| ['', '', ''], | |
| ['', '', ''], | |
| ], | |
| { | |
| align: ['', '', ''], | |
| }, | |
| ); | |
| }; | |
| // Atom definition | |
| const handsontableModalAtom = atom<HandsontableModalState>({ | |
| isOpened: false, | |
| }); | |
| // Read-only hook | |
| export const useHandsontableModalStatus = () => { | |
| const [state] = useAtom(handsontableModalAtom); | |
| return state; | |
| }; | |
| // Actions-only hook | |
| export const useHandsontableModalActions = () => { | |
| const setState = useSetAtom(handsontableModalAtom); | |
| const open = ( | |
| table: MarkdownTable, | |
| autoFormatMarkdownTable?: boolean, | |
| onSave?: HandsontableModalSaveHandler, | |
| ) => { | |
| setState({ | |
| isOpened: true, | |
| table, | |
| autoFormatMarkdownTable, | |
| onSave, | |
| }); | |
| }; | |
| const close = () => { | |
| setState({ | |
| isOpened: false, | |
| table: defaultMarkdownTable(), | |
| autoFormatMarkdownTable: false, | |
| onSave: undefined, | |
| }); | |
| }; | |
| return { open, close }; | |
| }; |
| const { close: closeHandsontableModalForEditor } = useHandsontableModalForEditorActions(); | ||
| const handsontableModalForEditorData = useHandsontableModalForEditorStatus(); | ||
| const isOpendInEditor = handsontableModalForEditorData.isOpened; | ||
| const editor = handsontableModalForEditorData?.editor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- どの hook 由来の変数か追いにくかったため view で使うものと editor で使うものを分けた
- .isOpened については jotai 化したタイミング nullable ではなくなったためオプショナルチェーンを外した
| } | ||
| else { | ||
| markdownTableState = table; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View で開く場合と Editor で開く場合で処理を分けた
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isOpenedInEditor じゃないかな (e が足りない)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました
|
|
||
| if (!isOpened) { | ||
| return <></>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今のままだとモーダルの fade エフェクトがなくなったりするという微妙なデメリットがある
ちゃんとやるなら他のモーダル (ShortcutsModal など) を参考に、一番外側の <Modal> は Container component (lightweight, always rendered) に含むように改修することになる
HandsontableModalSubstance は view / editor の場合分けなしに両対応するとして、
それをラップする HandsontableModal 側で場合分けすべきかなと思う
ちなみに DrawioModal も同じかもしれない
| } | ||
| else { | ||
| markdownTableState = table; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isOpenedInEditor じゃないかな (e が足りない)
| onClick={onClickTableButton} | ||
| > | ||
| <span className="material-symbols-outlined fs-5">table_chart</span> | ||
| <span className="material-symbols-outlined fs-5">table</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
| else { | ||
| markdownTableState = table; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました
|
|
||
| if (!isOpened) { | ||
| return <></>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ShortcutsModal を参考に修正しました。
View / Editor の場合分けは HandsontableModal で行い HandsontableModalSubstance では意識しなくて良いようにしました


Task
https://redmine.weseek.co.jp/issues/174842