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: add support for SSR environments #19

Merged
merged 3 commits into from
Jun 21, 2024
Merged

Conversation

tafelnl
Copy link
Member

@tafelnl tafelnl commented Jun 18, 2024

Closes #17

README.md Outdated Show resolved Hide resolved
@phlegx
Copy link

phlegx commented Jun 18, 2024

@tafelnl thank you for the PR! Nice! Please see my comment on README.md.

@phlegx
Copy link

phlegx commented Jun 19, 2024

@tafelnl can you release please?

@tafelnl
Copy link
Member Author

tafelnl commented Jun 19, 2024

I always try to get PRs tested before I merge them. If you would be able to help me with this and confirm it works correctly, I'll be able to merge it sooner

@phlegx
Copy link

phlegx commented Jun 19, 2024

It works perfectly! Call of initialize(); is not required as thought. This line does the job: https://github.com/capacitor-community/safe-area/pull/19/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R37

So, the initialize export, function call and function function initialize() { can be removed.

src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@tafelnl
Copy link
Member Author

tafelnl commented Jun 20, 2024

I am not sure if I understand what you're saying.

This line does the job: https://github.com/capacitor-community/safe-area/pull/19/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R37

Here you're saying that we should call initialize, but per your other comments it looks like you think it should be removed. That's confusing me.

Also, I'm pretty sure it's needed to call initialize as that will set the fallback css variables. If you have set those manually in your app, that's perfectly fine of course. But we should still provide a helper method for developers who are not doing that, or are not familiar with that even

@phlegx
Copy link

phlegx commented Jun 20, 2024

Hi @tafelnl!

Sorry for the confusion.

...but per your other comments it looks like you think it should be removed.

Yes, that was what I meant.

Also, I'm pretty sure it's needed to call initialize as that will set the fallback css variables. If you have set those manually in your app, that's perfectly fine of course. But we should still provide a helper method for developers who are not doing that, or are not familiar with that even

But anyway, we should provide the helper method as you suggested! The PR is therefore correct like it is.

@tafelnl
Copy link
Member Author

tafelnl commented Jun 20, 2024

Thanks for the clarification!

@tafelnl tafelnl merged commit e946cf9 into master Jun 21, 2024
@tafelnl tafelnl deleted the feat/add-ssr-support branch June 21, 2024 08:34
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.

Ignore property set if document is not present (SSR).
2 participants