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.

Sign up for GitHub

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

Jump to bottom

fix: ensure window width and height are defined #42

Merged
merged 3 commits into from
May 17, 2021
Merged

fix: ensure window width and height are defined #42

merged 3 commits into from
May 17, 2021

Conversation

helenamerk
Copy link
Contributor

My client was returning NaN for window.outerHeight and window.outerWidth, making the actor remove itself prematurely.

This change proposes falling back to document.documentElement.clientHeight and document.documentElement.clientWidth (as those returned correct values)

@helenamerk helenamerk changed the title fix: ensure window w and h are defined fix: ensure window width and height are defined May 16, 2021
JoshuaKGoldberg
JoshuaKGoldberg requested changes May 16, 2021
View reviewed changes
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

👋 @helenamerk, thanks for using Emojisplosion and sending this PR! I'm excited that you're using it. And Glimpse looks really cool 😄 .

Could you describe a bit more detail about what you mean by "My client was returning NaN for window.outerHeight and window.outerWidth"? If that's the case I would think we'd want to isNaN check here -- or if the case is that they're undefined, I'd be very interested to know what client/browser is causing that.

package.json Outdated Show resolved Hide resolved
src/actor.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label May 16, 2021
@helenamerk helenamerk force-pushed the master branch 4 times, most recently from 8a8e9eb to 62c1fc8 Compare May 17, 2021 00:15
@helenamerk
Copy link
Contributor Author

helenamerk commented May 17, 2021 •
edited

👋 @helenamerk, thanks for using Emojisplosion and sending this PR! I'm excited that you're using it. And Glimpse looks really cool 😄 .

Could you describe a bit more detail about what you mean by "My client was returning NaN for window.outerHeight and window.outerWidth"? If that's the case I would think we'd want to isNaN check here -- or if the case is that they're undefined, I'd be very interested to know what client/browser is causing that.

👋 hello @JoshuaKGoldberg !! Love this library. side note: Our team built out a few templates like "rocket" and "rainbow" that are super fun and might be neat additions to this project. Once we have some time we'll contribute more.

Not sure why our client is not respecting window.outerHeight and window.outerWidth, I tried everything to get around that first before submitting a PR haha. It's an embedded view view in a mac os app (which I believe fall back to safari on mac, chrome on windows). Definitely an edge case!

@JoshuaKGoldberg JoshuaKGoldberg self-requested a review May 17, 2021 00:29
JoshuaKGoldberg
JoshuaKGoldberg reviewed May 17, 2021
View reviewed changes
src/actor.ts Outdated Show resolved Hide resolved
JoshuaKGoldberg
JoshuaKGoldberg approved these changes May 17, 2021
View reviewed changes
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I'll merge this in now and release a new version.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 979069e into JoshuaKGoldberg:master May 17, 2021
@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request May 17, 2021
Closed
@JoshuaKGoldberg
Copy link
Owner

@helenamerk https://www.npmjs.com/package/emojisplosion/v/2.6.1 is now published. Cheers!

peterennis added a commit to peterennis/emojisplosion that referenced this pull request Aug 2, 2021
@peterennis
Merge pull request #2 from JoshuaKGoldberg/master
745864b 
fix: ensure window width and height are defined (JoshuaKGoldberg#42)
@JoshuaKGoldberg
Copy link
Owner

@allcontributors please add @helenamerk for bug. please add @helenamerk for code.

@allcontributors allcontributors bot mentioned this pull request May 27, 2023
Merged
@allcontributors
Copy link
Contributor

@JoshuaKGoldberg

I've put up a pull request to add @helenamerk! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
status: waiting for author Needs an action taken by the original poster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@helenamerk @JoshuaKGoldberg