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

Remove ToastPresentation usage and use toaster to display messages #3573

Merged
merged 28 commits into from
Jul 21, 2022

Conversation

veekeys
Copy link
Member

@veekeys veekeys commented May 2, 2022

Added iTwinUI toaster to display the toasts.
Returned old implementation on message components to get rid of ToastPresentation and marked them as deprecated.
Added new function to register ref to animate iTwinUI toasts to.
Added few deprecated tags as suggestions to some of managers. If we agree to all these deprecations (there might be more), I'd like to also add text in jsdoc about using toaster to display toasts where relevant.

@NancyMcCallB
Copy link
Contributor

Vykintas, this does not hook the toaster into the MessageManager -- MessageManager.outputMessage does not open any toasts. Is that still in progress or just broken?

@veekeys
Copy link
Member Author

veekeys commented May 3, 2022

Vykintas, this does not hook the toaster into the MessageManager -- MessageManager.outputMessage does not open any toasts. Is that still in progress or just broken?

Where have you found it's broken?
outputMessage has this 'ifs' carousel, which changes types or shows some other messages:
image

and then at the end it is calling addMessage which has the toaster hooked up:
image

Unless I misunderstood here something..?

@NancyMcCallB
Copy link
Contributor

Where have you found it's broken?

I pulled and built the branch and ran ui-test-app. The messages get logged in the message center, but no toast message show up, except activity messages.

@GerardasB
Copy link
Contributor

Good progress, that should cover most cases where toasts/messages are displayed.
As discussed, we need to ensure that ActivityMessage (and other message components) are not used internally (replaced with itwinui toaster for UI consistency). I.e. we probably want to use itwinui toaster in: ActivityMessagePopup, StatusMessagesContainer.
Also, specifically for ActivityMessage we need a component with equivalent layout that's rendered via the itwinui toaster.

@veekeys
Copy link
Member Author

veekeys commented May 26, 2022

@NancyMcCallB @GerardasB Would you mind checking again? I have fully replaced all toasts and added new custom activity message.
In component examples toasts are not visible, because iui-toast-wrapper is having too low z-index (edit that through console to see them).
THere is already new iTwinUI version with z-index fix, I just need to update here. Will creae a separate PR for this, I think.

If this approach looks ok, I will need to update with better description and maybe move files around?

@aruniverse
Copy link
Member

git merge master, rush clean, rush purge, rush update, rush build, rush extract-api

@veekeys
Copy link
Member Author

veekeys commented May 27, 2022

git merge master, rush clean, rush purge, rush update, rush build, rush extract-api

This helped! Thanks!
All of this should be in documentation, IMO.

@aruniverse
Copy link
Member

This helped! Thanks! All of this should be in documentation, IMO.

yea probably. thanks for volunteering 😜

@veekeys veekeys marked this pull request as ready for review June 1, 2022 10:39
@veekeys veekeys requested review from a team as code owners June 1, 2022 10:39
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
common/api/appui-react.api.md Outdated Show resolved Hide resolved
ui/appui-react/src/appui-react/statusbar/StatusBar.tsx Outdated Show resolved Hide resolved
@veekeys veekeys changed the title Remove ToastPresentation usage and use toaster to display toasts Remove ToastPresentation usage and use toaster to display messages Jul 21, 2022
@veekeys veekeys merged commit eb3d6af into master Jul 21, 2022
@veekeys veekeys deleted the vyki/toasts-refactor branch July 21, 2022 14:23
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.

5 participants