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

fix(form): show error at the end if inputs are not valid #398

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

NicoSerranoP
Copy link
Member

@NicoSerranoP NicoSerranoP commented Oct 15, 2024

Problem

The Form component uses pages for forms that have multiple sections. In each section the inputs already alert the user about some conditions (e.g.: you need a valid eth address, this field needs at least 3 words, etc).

The problem happens in the end. If the user ignored the warnings of each input in each section, then the form is not valid and cannot be submitted. When the user press the submit button nothing happens (no alert).

Solution

Detect if the form has any validation errors and tell the user that when he presses the submit button. That way the user will know he needs to trace back and find the input validation error.

Extras

  1. Delete "Applications" from top navbar
  2. "Create New Application" button in each round sends to the /rounds/[roundId]/new page

Copy link

vercel bot commented Oct 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
maci-platform ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 10, 2024 2:05am

Copy link
Contributor

@kittybest kittybest left a comment

Choose a reason for hiding this comment

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

Great! Thanks! Just thinking about some details:

  1. I think clicking the ImageUpload somehow trigger the submit function, so if there's any errors on the page, the error would be fixed at the bottom of the form, maybe check the button type of ImageUpload
  2. even after I fixed the errors (e.g. fix the length of the name) the error is stilly displayed
  3. just suggestion, thinking about changing those error reminder to other color rather than just black
  4. another suggestion, also thinking about move the errorMessage to alignRight under the submit buttons
  5. Also thinking about should we also check the restrictions before clicking Next?

Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@NicoSerranoP thanks, just one comment

packages/interface/src/components/ui/Form.tsx Outdated Show resolved Hide resolved
@NicoSerranoP
Copy link
Member Author

NicoSerranoP commented Oct 16, 2024

Great! Thanks! Just thinking about some details:

  1. I think clicking the ImageUpload somehow trigger the submit function, so if there's any errors on the page, the error would be fixed at the bottom of the form, maybe check the button type of ImageUpload
  2. even after I fixed the errors (e.g. fix the length of the name) the error is stilly displayed
  3. just suggestion, thinking about changing those error reminder to other color rather than just black
  4. another suggestion, also thinking about move the errorMessage to alignRight under the submit buttons
  5. Also thinking about should we also check the restrictions before clicking Next?
  • 1. I think it is not doing that anymore. Could you check again? 😊
  • 2. I used a timer that will delete the message after 5s (I cannot detect the changes of the previous page form without drastically changing the component instruction)
  • 3. should we use red in all <ErrorMessage> ?
  • 4. Done
  • 5. Blocked: I don't know how to do this in a generic mode (the steps and pages are set in each specific form component)

@kittybest
Copy link
Contributor

Great! Thanks! Just thinking about some details:

  1. I think clicking the ImageUpload somehow trigger the submit function, so if there's any errors on the page, the error would be fixed at the bottom of the form, maybe check the button type of ImageUpload
  2. even after I fixed the errors (e.g. fix the length of the name) the error is stilly displayed
  3. just suggestion, thinking about changing those error reminder to other color rather than just black
  4. another suggestion, also thinking about move the errorMessage to alignRight under the submit buttons
  5. Also thinking about should we also check the restrictions before clicking Next?
  • 1. I think it is not doing that anymore. Could you check again? 😊
  • 2. I used a timer that will delete the message after 5s (I cannot detect the changes of the previous page form without drastically changing the component instruction)
  • 3. should we use red in all <ErrorMessage> ?
  • 4. Done
  • 5. Blocked: I don't know how to do this in a generic mode (the steps and pages are set in each specific form component)

This 5th one is not required, just a suggestion, no need to spend much time on it.
And for the condition mentioned in 1st one, let me add a video:
https://drive.google.com/file/d/18jyLWRraw03LilH_H2KstTuCEfVG7HwF/view?usp=sharing

@NicoSerranoP
Copy link
Member Author

@kittybest @0xmad @ctrlc03 I fixed two ui issues that are being addressed at #407 as well. I think the only component that would give us conflict when merging that branch is ../interface/src/features/applications/components/ApplicationsToApprove.tsx :)

Copy link
Contributor

@kittybest kittybest left a comment

Choose a reason for hiding this comment

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

Currently lgtm, thanks a lot! plz rebase and fix the conflicts

@NicoSerranoP
Copy link
Member Author

I think we need @0xmad 's approval for the code changes request
image

@kittybest
Copy link
Contributor

@NicoSerranoP There's still prettier and commitlint check not passed.
So you need to:

  1. run pnpm lint:ts in the package first
  2. squash the commits, that there wouldn't be any Merge branch 'main' into fix/validation-errors-in-form anymore, which is causing the commitlint error.
  3. to merge main branch in the future, please use rebase instead of merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants