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

Timer in prompt form #70

Merged
merged 7 commits into from
Jan 6, 2025
Merged

Timer in prompt form #70

merged 7 commits into from
Jan 6, 2025

Conversation

chewmanji
Copy link
Member

@chewmanji chewmanji commented Jan 3, 2025

  • added lastRequestTimestamp variable to local storage to track when the user sent last request - it's needed to calculate time required to wait for the next request
  • added custom hook for ease of use - converting Date to string and vice versa
  • main logic happens in useEffect in PromptForm component where I check if timestamp is already in local storage, calculate delta to decide if button should be enabled and update the lock duration in interval - it happens in useEffect since we need to have access to browser local storage
  • in current solution calculating time is not really precise since setInterval is not accurate - if you know better solution feel free to share it
  • added env variable for "rate limitting" in frontend
  • added tooltip

How it looks:
image

@chewmanji chewmanji self-assigned this Jan 3, 2025
@chewmanji chewmanji added the enhancement New feature or request label Jan 3, 2025
@chewmanji chewmanji linked an issue Jan 3, 2025 that may be closed by this pull request
@chewmanji chewmanji marked this pull request as ready for review January 3, 2025 17:41
@chewmanji chewmanji requested a review from maciejkrol18 January 3, 2025 17:41
Copy link
Member

@maciejkrol18 maciejkrol18 left a comment

Choose a reason for hiding this comment

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

Good job overall

src/components/prompt-form.tsx Show resolved Hide resolved
Comment on lines 176 to 188
<TooltipProvider delayDuration={0}>
<Tooltip>
<TooltipTrigger>
<span className="text-sm">Odczekaj: {lockDuration}</span>
</TooltipTrigger>
<TooltipContent>
<span>
Ograniczenie jest wprowadzone dla zachowania stabliności
systemu
</span>
</TooltipContent>
</Tooltip>
</TooltipProvider>
Copy link
Member

@maciejkrol18 maciejkrol18 Jan 6, 2025

Choose a reason for hiding this comment

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

suggestion: I think we can get the message through with less text upfront. I would add a clock icon with just the seconds remaining next to it, and provide more information only on hover. I think that this being a timeout countdown should be intuitive to majority of users, and this way it looks nicer imo

image

Suggested change
<TooltipProvider delayDuration={0}>
<Tooltip>
<TooltipTrigger>
<span className="text-sm">Odczekaj: {lockDuration}</span>
</TooltipTrigger>
<TooltipContent>
<span>
Ograniczenie jest wprowadzone dla zachowania stabliności
systemu
</span>
</TooltipContent>
</Tooltip>
</TooltipProvider>
<TooltipProvider delayDuration={0}>
<Tooltip>
<TooltipTrigger className="flex items-center gap-x-1 w-12 text-red-500">
<ClockAlert className="size-4"/>
<span>{lockDuration}</span>
</TooltipTrigger>
<TooltipContent className="text-center">
<span>
Odczekaj przed wysłaniem kolejnego żądania
<br/>
Ograniczenie jest wprowadzone dla zachowania stabilności systemu
</span>
</TooltipContent>
</Tooltip>
</TooltipProvider>

Notes: fixed width on TooltipTrigger ensures that there's no slight layout shifts when the numbers change

Copy link
Member Author

Choose a reason for hiding this comment

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

Notes: fixed width on TooltipTrigger ensures that there's no slight layout shifts when the numbers change

PRly dose of CSS goes here 😂

@chewmanji chewmanji merged commit 8629615 into main Jan 6, 2025
1 check passed
@chewmanji chewmanji deleted the feat/66-timer-in-prompt-form branch January 6, 2025 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add timer in prompt form
2 participants