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

feat: don't hardcode toast duration #107

Merged
merged 2 commits into from
Oct 28, 2022
Merged

feat: don't hardcode toast duration #107

merged 2 commits into from
Oct 28, 2022

Conversation

njenwei
Copy link
Contributor

@njenwei njenwei commented Oct 27, 2022

Problem

Update useToast component to accept a duration param.

@netlify
Copy link

netlify bot commented Oct 27, 2022

Deploy Preview for objective-bell-0ffbfb canceled.

Name Link
🔨 Latest commit a43995d
🔍 Latest deploy log https://app.netlify.com/sites/objective-bell-0ffbfb/deploys/635b73a8ab19620008ab9a78

@njenwei njenwei requested a review from karrui October 27, 2022 18:02
@@ -36,7 +36,7 @@ export const useToast = ({

const customToastImpl = useMemo(() => {
const impl = ({
duration = 6000,
duration = initialProps.duration,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use initial duration props too for consistency?

Should we also add back the default value to prevent duration changes due to this new prop assignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set the default value via param defaults. Had to rename it to initialDuration, similar to initialStatus

duration: initialDuration = 6000

@njenwei njenwei requested a review from karrui October 28, 2022 06:28
...initialProps
}: UseToastProps = {}): UseToastReturn => {
const toast = useChakraToast(initialProps)

const customToastImpl = useMemo(() => {
const impl = ({
duration = 6000,
duration = initialDuration,
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually this doesn't matter right? this is just a function that we return in useTOast. the user using useToast can override the props themselves (since it is just a function) don't need this PR at all right?

@njenwei njenwei merged commit a52c899 into main Oct 28, 2022
@njenwei njenwei deleted the feat/toast-duration branch October 28, 2022 06:57
njenwei added a commit that referenced this pull request Oct 28, 2022
* fix: update vite config to fix downstream build (#103)

* fix: update dropdown component to be more reliable (#106)

* fix: update dropdown component to be more reliable

Linked to a bunch of fixes in the upstream FormSG repository, see:
opengovsg/FormSG#5192
opengovsg/FormSG#5137
opengovsg/FormSG#4922
opengovsg/FormSG#4778

* fix: remove unused dependencies

* feat: don't hardcode toast duration (#107)

* feat: dont hardcode toast duration

* feat: set default toast duration

* chore: bump package version to 0.0.13-beta.5

Co-authored-by: Kar Rui Lau <[email protected]>
@karrui karrui mentioned this pull request May 25, 2023
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