Skip to content

Conversation

@brenno-silva
Copy link
Contributor

Description

Added form validation and auto modal pop up when there is an error

Resolves (Issues)

#367

General tasks performed

  • Added form validation
  • Added function to suto show modal when there is an error

Have you confirmed the application builds locally without error? See here.

  • Yes

Comment on lines 123 to 124
// 54 is the length when there is no problem
if (artworkError.length != 54) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be better if you compare directly the success string here instead of the length of the response.
Something like artworkError != "success", since using the length makes it very obscure for maintenance, I'm not even sure what text will be shown if there is no error, only that it has "54" characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was strugling to find a proper conditional and that length was the only thing that worked. I changed to check if the innerTexts have part of the error messages

Comment on lines 129 to 130
// 56 and 62 are the length when there is no problem
else if (exhibitNameError.length != 56 || exhibitSlugError.length != 62) {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing as stated above, using the length here is a bad idea for maintenance, can we compare the strings directly?

You can create boolean variables to store the results and make it even clearer.
Eg:

noExhibitNameError = exhibitNameError != "success with 56 chars"
...
else if noExhibitNameError {

@pablodiegoss pablodiegoss mentioned this pull request Feb 10, 2025
1 task
@pablodiegoss pablodiegoss merged commit 51a08c4 into memeLab:develop Feb 10, 2025
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.

2 participants