Skip to content

refactor: change form handler on ExportOptionsModal#4207

Open
Sebastien-Ahkrin wants to merge 10 commits into
mainfrom
4172-refactor-useform-on-exportoptionsmodal
Open

refactor: change form handler on ExportOptionsModal#4207
Sebastien-Ahkrin wants to merge 10 commits into
mainfrom
4172-refactor-useform-on-exportoptionsmodal

Conversation

@Sebastien-Ahkrin

Copy link
Copy Markdown
Collaborator

Closes: #4172

@Sebastien-Ahkrin Sebastien-Ahkrin linked an issue Jun 18, 2026 that may be closed by this pull request
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 18, 2026

Copy link
Copy Markdown

Deploying nmrium with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4ee3e06
Status: ✅  Deploy successful!
Preview URL: https://fc2cae2c.nmrium.pages.dev
Branch Preview URL: https://4172-refactor-useform-on-exp.nmrium.pages.dev

View logs

@targos

targos commented Jun 19, 2026

Copy link
Copy Markdown
Member

I suggest you take care of #4215 in a separate PR to unblock this.

@Sebastien-Ahkrin Sebastien-Ahkrin requested review from targos and tpoisseau and removed request for targos June 23, 2026 12:49
@Sebastien-Ahkrin Sebastien-Ahkrin marked this pull request as ready for review June 23, 2026 12:49

@tpoisseau tpoisseau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Advanced Mode looks broken

Image

Height, Width and Unit are empty.

This forms reminds me this one:

CleanShot 2026-06-24 at 09 31 30@2x

I suggest you to refactor / re-use part of this form or copy-paste it. It is working properly even in advanced mode.

src/component/modal/setting/tanstack_general_settings/tabs/export_tab.fields.tsx

I hope the data-struct is the same so you can simply re-use the field-group.
This one also use setFieldValue, but with dontRunListeners: true option. without it, editing values was slow, editing width edit height, and height edit the width. it loop until conversion between both was stable (or tanstack have a kill-switch to stop this kind of infinite edit-loop). So I predict your current implementation have this issue.

@Sebastien-Ahkrin

Copy link
Copy Markdown
Collaborator Author

Advanced Mode looks broken

Image Height, Width and Unit are empty.

This forms reminds me this one:

CleanShot 2026-06-24 at 09 31 30@2x I suggest you to refactor / re-use part of this form or copy-paste it. It is working properly even in advanced mode.

src/component/modal/setting/tanstack_general_settings/tabs/export_tab.fields.tsx

I hope the data-struct is the same so you can simply re-use the field-group. This one also use setFieldValue, but with dontRunListeners: true option. without it, editing values was slow, editing width edit height, and height edit the width. it loop until conversion between both was stable (or tanstack have a kill-switch to stop this kind of infinite edit-loop). So I predict your current implementation have this issue.

We do not have the save data-struct, so i can't re-use the fieldgroup. I think, to keep the layout as it i will not use the implementation of general settings. (keep the link between width / height etc). i will use dontRunListeners then.

@tpoisseau

Copy link
Copy Markdown
Contributor

It should be the same data-struct. Or at least compatible with encode or decode of exportSettingsValidation

/**
* @see {import("@zakodium/nmrium-core").ExportSettings}
*/
export const exportSettingsValidation = z.discriminatedUnion('mode', [
basicExportSettings,
advancedExportSettings,
]);

InnerExportOptionsModal form fields should be exactly the fields from ExportFields

Both should looks and behave exactly the same. Both forms edit ExportSettings from the workspace.

@tpoisseau tpoisseau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⬆️

Comment thread src/component/elements/export/ExportOptionsModal.tsx Outdated
Comment thread src/component/elements/export/ExportOptionsModal.tsx Outdated
Comment thread src/component/elements/export/ExportOptionsModal.tsx
@Sebastien-Ahkrin

Copy link
Copy Markdown
Collaborator Author
CleanShot 2026-06-25 at 15 02 16 CleanShot 2026-06-25 at 15 02 20

@tpoisseau tpoisseau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you took into account my last comment, you are good to merge.

<StyledDialogBody>
<ExportFields form={form} fields="values" />
</StyledDialogBody>
<DialogFooter>

@tpoisseau tpoisseau Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not notice before your screens but you should use actions props of DialogFooter instead of children.

children is for extra content, left align. actions is for, well, actions, right align.

Also, actions take care of align and spacing. You just have to put a fragment with Buttons inside in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor useForm on ExportOptionsModal

3 participants