-
Notifications
You must be signed in to change notification settings - Fork 42
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
Designed a Toast Component #70
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Left some suggestions.
Also appreciate the formatted commit message.
(But do mention that the App component is wrapped by the toast provider)
import './toast.css'; | ||
|
||
function useTimeout(callback: () => void, duration: number) { | ||
const savedCallback = useRef(callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using useCallback
would simplify the code and would make lexical sense. It would also help get rid of the useEffect.
In most cases though, there won't be any benefit of caching the callbacks since they will mostly contain state setters, which update on every re-render, thus re-rendering the entire toast component. But we'll cache nonetheless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuv2707 i did not implement this suggestion as i didn't understand how we could have avoided the use useEffect effectively and i was concerned that it might break the code . Nonetheless this still works and its implementation can be done in the future
frontend/src/library/toast/Toast.tsx
Outdated
return () => { | ||
clearTimeout(functionId); | ||
}; | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please populate the array to get rid of the eslint error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuv2707 done with this
close: (id: number) => void; | ||
}; | ||
|
||
export const ToastContext = createContext<ToastContextValue | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The | null
causes the return value of useToast
to be ToastContextValue | null, thus making us check for the null case wherever we use it. We can just use empty functions as default values for open and close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuv2707 done with this too
frontend/src/library/toast/Toast.tsx
Outdated
|
||
function closeToast(id: number) { | ||
setToasts((prevToasts) => | ||
prevToasts.filter((toast) => toast.id !== id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes the toast to abruptly disappear. We'd want a fade-out animation or sth of that sort. You can put the filter part inside a useTimeout, and just add a class for fade-out animation. This will cause the component to fade out, and after that the timeout kicks in, removing it entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuv2707 checkout the closing animation video
const contextValue = useMemo( | ||
() => ({ | ||
open: openToast, | ||
close: closeToast, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to have some default values, like INFO
, ERROR
etc. for the toast, so that we don't always have to pass in the colors etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuv2707 the color parameter in the open function accepts only 4 values : info,error,warning and success not any particular color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed it now, looking closely at the screenshot.
In that case you can change the type of color
to be a union of these 4 strings.
That way, it would be apparent by looking at the type itself, and we most probably won't want to customize the color anyway.
To further improve usability, you can take the arguments in an object, so that when using the open function, we only specify the values we wish to change without being concerned about the order, and use the defaults for the rest.(You can choose the defaults to be {"",5 seconds, center, info}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuv2707 done with this too
@ritwik-69 This can be merged once the minor changes suggested are applied. |
This commit creates the toast component with customizable position,duration and color. In main.tsx wrapped the app component in toastcontextprovider so that toast can be used everywhere in the app. fixes:shivansh-bhatnagar18#68
closing animation closing.animation.1.1.mp4 |
There are a few polishing issues with the implementation, but I'll fix them myself. |
This commit creates the toast component with customizable position,duration and color.
fixes:#68
Description
Created the toast component with customizable position,duration and color.To use you need use the custom hook useToast()
and then toast.open(message,duration,position,color) to get the toast at that desired position.
Motivation and Context
[Explain the motivation behind these changes and provide any relevant context.]
How to Test
[Describe the steps to test the changes made in this pull request.]
Related Issues
[If applicable, mention any related issues or pull requests.]
Checklist
Screenshots (if applicable)
[If your changes include any visual updates, provide screenshots here.]
toast.vid.1.mp4