GetJobber / atlantis

🔱 Atlantis
https://atlantis.getjobber.com
MIT License
25 stars 30 forks source link

feat(components): SSR support for toast #1948

Closed MichaelParadis closed 2 months ago

MichaelParadis commented 3 months ago

Motivations

During our SSR support updates we missed adding support for toasts. This PR fixes that by preventing references to document if it is undefined

Changes

Added

Changed

Deprecated

Removed

Fixed

Security

Testing

Verify Toasts work in the referenced PRs

Changes can be tested via Pre-release


In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

github-actions[bot] commented 3 months ago

Published Pre-release for 4b12bdf465f6d1e8abc0ef645c19dc71b2b9ad27 with versions:

  - @jobber/components@5.17.1-MIKEssr-s.4+4b12bdf4

To install the new version(s) for Web run:

npm install @jobber/components@5.17.1-MIKEssr-s.4+4b12bdf4
cloudflare-workers-and-pages[bot] commented 3 months ago

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4b12bdf
Status: âœ…  Deploy successful!
Preview URL: https://811ce743.atlantis.pages.dev
Branch Preview URL: https://mike-ssr-support-for-toast.atlantis.pages.dev

View logs

darryltec commented 3 months ago

Code looks good. Mind testing it in remix where you render it in the server. I wonder if that shows up on the client after mount or not at all until you fire it in the client again

MichaelParadis commented 3 months ago

@darryltec I have a referenced PR. I had to fire it again on the client since I wasn't able to find a way to modify the document while hydrating (I could have easily missed something)

scotttjob commented 2 months ago

should also export this in here packages/components/src/index.tsx so you can import from @jobber/components directlly

Only if this has been tested elsewhere to make sure it doesn't break existing implementations. That's why it was left out to begin with.

MichaelParadis commented 2 months ago

@scotttjob I assume it was left out initially because toast would break on SSR usages. However since this fixes that it should be good. I will do another pre-release and verify

scotttjob commented 2 months ago

@scotttjob I assume it was left out initially because toast would break on SSR usages. However since this fixes that it should be good. I will do another pre-release and verify

That's correct. However when I fixed the SSR issues in a last minute effort when I realized they were broken, it caused issues in other repos. So just make sure it's gravy everywhere and we should be good to go.

MichaelParadis commented 2 months ago

That's correct. However when I fixed the SSR issues in a last minute effort when I realized they were broken, it caused issues in other repos. So just make sure it's gravy everywhere and we should be good to go.

I was able to verify that this worked in the other places 😉

MichaelParadis commented 2 months ago

@darryltec updated in 4b12bdf465f6d1e8abc0ef645c19dc71b2b9ad27