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: rework toasts api and add mini mode support in toasts #987

Merged
merged 25 commits into from
Feb 29, 2024

Conversation

sachin-into
Copy link
Collaborator

No description provided.

Copy link

netlify bot commented Feb 22, 2024

Deploy Preview for testitori ready!

Name Link
🔨 Latest commit 26e1b4c
🔍 Latest deploy log https://app.netlify.com/sites/testitori/deploys/65e07056cbf1e80009112133
😎 Deploy Preview https://deploy-preview-987--testitori.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 22, 2024

Deploy Preview for teritori-dapp ready!

Name Link
🔨 Latest commit 26e1b4c
🔍 Latest deploy log https://app.netlify.com/sites/teritori-dapp/deploys/65e070560771250008952ede
😎 Deploy Preview https://deploy-preview-987--teritori-dapp.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

we already have this kind of systems in useFeedback, please adapt it instead of having two providers and api for feedback

@n0izn0iz n0izn0iz changed the title Feat : Toast component for mini mode feat: Toast component for mini mode Feb 22, 2024
Copy link
Collaborator

@omniwired omniwired left a comment

Choose a reason for hiding this comment

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

looks good, I agree with @n0izn0iz about useFeedback

Comment on lines 56 to 57
miniToast: { message: "" },
setMiniToast: () => {},
Copy link
Collaborator

@n0izn0iz n0izn0iz Feb 23, 2024

Choose a reason for hiding this comment

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

don't add new api, use previous api or extend it and change the toast render only
we need all existing toasts to work in mini mode also

Copy link
Collaborator Author

@sachin-into sachin-into Feb 26, 2024

Choose a reason for hiding this comment

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

mini toast has 4 different status and 2 different variant style so I created new api without touching previous working apis. Previous toast works as it used to and new api is for mini mode. Won't that be simple and easier to distinguish?

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think, a toast should be a toast, the mode can affect how it renders but we should have a single api

you can extend the current api

topOffset?: number;
}

export const NormalToast: React.FC<NormalToastProps> = ({
Copy link
Collaborator

@n0izn0iz n0izn0iz Feb 28, 2024

Choose a reason for hiding this comment

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

there is a regression in this component, on main it has an icon

Screenshot 2024-02-28 at 12 20 47

on this pr the ! icon does not appear

Screenshot 2024-02-28 at 12 23 13

Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

regression in padding
this pr:
Screenshot 2024-02-29 at 11 51 26
main:
Screenshot 2024-02-29 at 11 53 34

@n0izn0iz n0izn0iz changed the title feat: Toast component for mini mode feat: rework toasts api and add mini mode support in toasts Feb 29, 2024
Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

LGTM :)

@n0izn0iz n0izn0iz merged commit 8b87fd1 into main Feb 29, 2024
19 checks passed
@n0izn0iz n0izn0iz deleted the feat/mini-Toast branch February 29, 2024 15:21
clegirar pushed a commit that referenced this pull request Mar 20, 2024
* feat:created toast component for mini mode and added icons and colors required for it and, created ToastProvider component

* refactor: moved toast message clear logic from toast component to toast context provider

* refactor: replaced MiniToast on mini comment input

* refactor: removed ToastProvider and moved added miniToast on feedback provider

* chore: removed console.log

* feat: added clear toast on press

* refactor: Feedback Provider to use single api for mini and normal toast,
removed: ErrorToast and SuccesToast to make single NormalToast,
deprecated use of setToastError and setToastSuccess

* fix: tsc errors

* fix: unused exports error

* feat:created toast component for mini mode and added icons and colors required for it and, created ToastProvider component

* refactor: moved toast message clear logic from toast component to toast context provider

* refactor: replaced MiniToast on mini comment input

* refactor: removed ToastProvider and moved added miniToast on feedback provider

* chore: removed console.log

* feat: added clear toast on press

* refactor: Feedback Provider to use single api for mini and normal toast,
removed: ErrorToast and SuccesToast to make single NormalToast,
deprecated use of setToastError and setToastSuccess

* fix: tsc errors

* fix: unused exports error

* fix: regression

* fix: regression no right padding

* fix:padding diffrenece in success and error

* fix: different with for success and error as in main

---------

Co-authored-by: Sakul Budhathoki <[email protected]>
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.

4 participants