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: DateSelector selector validation message issues #823

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

YuvalMasada
Copy link
Collaborator

@NoamGaash - This time at the right direction
I've added 2 short fixes.

  1. For wrong validation message I added a different message for the specific case. The message will pop for min/max, but I think that the max message will never appear( so maybe it would be better to delete it? not 100% sure ).
  2. To make the message re-render when language changed I've extend the useMemo dependency array with t function that should be reassign as the DateSelector component been re-rendered.

@YuvalMasada YuvalMasada requested a review from NoamGaash as a code owner June 19, 2024 06:03
Copy link
Contributor

github-actions bot commented Jun 19, 2024

@@ -20,7 +21,7 @@ export function DateSelector({ time, onChange, customLabel, minDate }: DataAndTi
return ''
}
}
}, [error])
}, [error, t])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better if we'll just remove the useMemo hook

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@NoamGaash
It's a good idea to remove it.
while removing it I had thought about 2 how to implement it.

  1. Just removing the useMemo and use the function.
  2. Take the function out of the component( and remove the useTranslation also) - will look better.
    does it matter which approach I go forward with? I did both of them and it works.

Copy link
Member

Choose a reason for hiding this comment

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

That's up to you
I'd like to make a third suggestion -

  1. the function could return the translation key
  2. the function can be inside the scope of the component, or underneath

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

I made a suggestion - it will let you keep using useTranslation which is better approach as it will trigger re-render on language change

@YuvalMasada
Copy link
Collaborator Author

@NoamGaash
At first I did something like your suggestion, but then typescript got mad at me.
It doesn't like that getErrorMessage can return undefined( the t function we got from the useTranslation hook).
I'll be happy to understand why typescript complain about it.

@NoamGaash
Copy link
Member

@YuvalMasada I'll try

@YuvalMasada YuvalMasada merged commit 7beccc4 into main Jul 17, 2024
16 of 17 checks passed
@YuvalMasada YuvalMasada deleted the fix/DateSelector_selector_validation_message_issues branch July 17, 2024 07:08
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