Skip to content

Feat/new toast #1696

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

Merged
merged 25 commits into from
Dec 20, 2021
Merged

Feat/new toast #1696

merged 25 commits into from
Dec 20, 2021

Conversation

ethanshar
Copy link
Collaborator

@ethanshar ethanshar commented Nov 26, 2021

Description

Please Review - Don't Merge!
This contains lots of files, most of them are assets and generated types. It'll be best to review this PR by commits

This is the new implementation of our Toast component (taken from the private lib)
The component's code has been refactored to a functional component
Im using separate helper hooks for different parts of logic of the component.

Few alterations I did

  • I added support for offline preset for the private usage we have (in private we deepened on the isConnected prop)
  • I added support for containerStyle, messageStyle, iconColor to give more customize capabilities to our usets

Changelog

New Toast component (under Incubator) with better UI and lots of new features

@ethanshar ethanshar requested a review from Inbal-Tish November 29, 2021 20:21
Copy link
Contributor

@lidord-wix lidord-wix 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, the hooks are making a lot of sense :) I left few specific comments.
The following are more generic:

  1. It might be a bug in the example screen, but when dismissing the toast by sliding it down or up, you should press the Toggle toast button 3 times to present it again (instead of 1).
  2. There's a bug in the attachment feature, you can see it when sliding the toast right/left (happens on private as well).
  3. I think it is a better API to have a customContent prop instead of getting it as children, it'll also be more clear to us in the future.
  4. Consider exporting the toast presets so users will be able to use Toast.presets.SUCCESS.

@lidord-wix
Copy link
Contributor

Looks good, the hooks are making a lot of sense :) I left few specific comments. The following are more generic:

  1. It might be a bug in the example screen, but when dismissing the toast by sliding it down or up, you should press the Toggle toast button 3 times to present it again (instead of 1).
  2. There's a bug in the attachment feature, you can see it when sliding the toast right/left (happens on private as well).
  3. I think it is a better API to have a customContent prop instead of getting it as children, it'll also be more clear to us in the future.
  4. Consider exporting the toast presets so users will be able to use Toast.presets.SUCCESS.

@ethanshar did you fix these issues?

@ethanshar
Copy link
Collaborator Author

Looks good, the hooks are making a lot of sense :) I left few specific comments. The following are more generic:

  1. It might be a bug in the example screen, but when dismissing the toast by sliding it down or up, you should press the Toggle toast button 3 times to present it again (instead of 1).
  2. There's a bug in the attachment feature, you can see it when sliding the toast right/left (happens on private as well).
  3. I think it is a better API to have a customContent prop instead of getting it as children, it'll also be more clear to us in the future.
  4. Consider exporting the toast presets so users will be able to use Toast.presets.SUCCESS.

@ethanshar did you fix these issues?

Not all, I'll go over them again.. thanks

@ethanshar
Copy link
Collaborator Author

Looks good, the hooks are making a lot of sense :) I left few specific comments. The following are more generic:

  1. It might be a bug in the example screen, but when dismissing the toast by sliding it down or up, you should press the Toggle toast button 3 times to present it again (instead of 1).
  2. There's a bug in the attachment feature, you can see it when sliding the toast right/left (happens on private as well).
  3. I think it is a better API to have a customContent prop instead of getting it as children, it'll also be more clear to us in the future.
  4. Consider exporting the toast presets so users will be able to use Toast.presets.SUCCESS.

@ethanshar did you fix these issues?

Not all, I'll go over them again.. thanks

@lidord-wix

  1. The first issue is more of an example screen issue, anyway I fixed it, you can take a look.
  2. Fixed
  3. I prefer not to change API, I copied the code the private and I'd like to have a smooth transition to the pubic implementation without having a big migration
  4. Done!

@ethanshar ethanshar merged commit cfa7c92 into master Dec 20, 2021
@ethanshar ethanshar deleted the feat/NewToast branch June 13, 2022 08:16
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