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

refactor: use native validation for parameter form #3349

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

TrySound
Copy link
Member

@TrySound TrySound commented May 13, 2024

Here switched to native html solution for validation in parameter variable form. Added three new form utilities to replicate existing behavior with closing errored form only on the second attempt when all errors are shown.

  • Form
  • useFormField
  • checkCanRequestSubmit

These are a few important apis for validation

  • form.requestSubmit()
  • form.checkValidity()
  • input.checkValidity()
  • input.setCustomValidity('message')
image

Here switched to native html solution for validation
in parameter variable form. Added three new form utilities
to replicate existing behavior with closing errored form only
on the second attempt when all errors are shown.

- Form
- useFormField
- checkCanRequestSubmit
@TrySound TrySound requested review from kof and istarkov May 13, 2024 11:05
};
};

export const useFormField = ({
Copy link
Member

@kof kof May 13, 2024

Choose a reason for hiding this comment

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

  1. I am confused about the difference between useFormField and useField
  2. shouldn't those be components?
  3. shoudln't those be in the design system package?

Copy link
Member Author

Choose a reason for hiding this comment

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

useFormField relies on native validation,
useField is old implementation, git did not diffed renaming well

Copy link
Member

Choose a reason for hiding this comment

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

then we need to call it useFieldDeprecated?

Copy link
Member

Choose a reason for hiding this comment

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

just so when we touch deprecated code, we migrate it to the new one

Copy link
Member

Choose a reason for hiding this comment

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

I saw the @deprecated thing, but I think its not visible enough to really incentivice to refactor

Copy link
Member Author

Choose a reason for hiding this comment

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

visible enough, we use it only in variable popover which I'm gonna rebuild anyway

@kof
Copy link
Member

kof commented May 13, 2024

  • Looks like its not saving on field blur
image

@TrySound
Copy link
Member Author

It never saved on blur

@kof
Copy link
Member

kof commented May 13, 2024

  • when changing the field then hitting ESC, do you think we should just revert the change, then on second ESC close the panel? Right now ESC acts as "save".

@kof
Copy link
Member

kof commented May 13, 2024

It never saved on blur

Lets get the desired behavior here? without waiting for a second PR?

@TrySound
Copy link
Member Author

It never saved on blur because we use the same forms for "create" and "edit". Form will loose focus if save on blur.

@TrySound
Copy link
Member Author

when changing the field then hitting ESC, do you think we should just revert the change, then on second ESC close the panel? Right now ESC acts as "save".

What change to revert if you want to save on blur?

@kof
Copy link
Member

kof commented May 13, 2024

It never saved on blur because we use the same forms for "create" and "edit". Form will loose focus if save on blur.

Maybe we need to differentiate the 2 cases? Because it feels much better when updating stuff when it applies right away and indeed when creating new stuff, you don't want it to create before you actively said you are done.

@kof
Copy link
Member

kof commented May 13, 2024

What change to revert if you want to save on blur?

Its tricky. When I changed something and it didn't save right away - then I am probably expecting it with ESC to not save it at all. When I changed it but it was visibly auto-saved (I can see it saved), then ESC is probably just expected to close the thing, wdyt? We can compare with some other UX best practices. This is my hunch

@kof
Copy link
Member

kof commented May 13, 2024

My biggest beef with our current behavior in some cases is that when I change a value and it is represented somewhere else - I don't see it being updated, I feel like if I close it, it will be gone, there is no clarity what happens if I close it now.

When everything is auto-saved all the time and reflected immediately everywhere - I feel most safe, but there are cases like you mentioned where I am creating a new item and don't expect it to be created until I do something confirmational like hitting ENTER

@kof
Copy link
Member

kof commented May 13, 2024

Did some testing with figma on behavior https://share.descript.com/view/8ETeg4IxS21

@kof
Copy link
Member

kof commented May 13, 2024

What change to revert if you want to save on blur?

when something was changed in the input and it wasn't autosaved, hitting ESC is basically like a preventDefault() for the blur that comes with hitting ESC, where blur would normally save, but since we hit ESC, blur won't save

@TrySound TrySound marked this pull request as draft May 16, 2024 18:45
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.

None yet

3 participants