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

WIP Detect browser for real max element size #936

Merged
merged 1 commit into from
Jan 13, 2018

Conversation

TrySound
Copy link
Collaborator

@TrySound TrySound commented Jan 1, 2018

Ref #932
Added naive detectors for chrome, safari, opera and firefox. Can't test edge since I'm on mac, so use default on it. Also didn't test mobile devices. Sadly didn't find info about this with googling.

@TrySound
Copy link
Collaborator Author

TrySound commented Jan 1, 2018

Boilerplate for checking

<div style="width: 1.0e+10px; height: 1.0e+10px;"></div>
<script>
const element = document.querySelector('div');
console.log(element.offsetWidth, element.offsetHeight, element.offsetWidth === element.offsetHeight);
</script>

Repository owner deleted a comment from codecov-io Jan 13, 2018
@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

Hey, this is cool. 😄 Does it work? Is it solid enough to release?

@TrySound
Copy link
Collaborator Author

Not sure. Browser testers I just googled. Maybe we can use some vendors for this.

@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

Just curious if you'd smoke tested this and felt like it worked well enough to release.

I like it. I've been meaning to do something like it for a while. I'd love to release it if it's stable enough (eg won't break SSR, has a fallback for IE+Edge, etc)

@TrySound
Copy link
Collaborator Author

Didn't try to write SSR test (without jsdom). ie/edge will have default which is already used.

@TrySound TrySound changed the title Detect browser for real max element size WIP Detect browser for real max element size Jan 13, 2018
Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I'm feeling bold. 😄 Let's try it.

@bvaughn bvaughn merged commit a0d5d59 into master Jan 13, 2018
@bvaughn bvaughn deleted the browser-specific-element-size branch January 13, 2018 22:07
@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

Ah, shit. I should have tested it a little more before merging.

Set the "Num rows" to "10000000" for the List demo page and scroll to the bottom:
screen shot 2018-01-13 at 2 10 23 pm

@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

Testing more locally, it looks like the max safe value for Chrome (before borders start to blur) is actually closer to 1.67771e+07. This value renders well in Safari too, although drag-scrolling this many pixels in either Safari or Firefox is really bad for perf. Firefox doesn't render reliably anywhere near this high it seems. Borders are arbitrarily darker or thinner at random. 😦 I think I'm going to have to revert this change for now. Or maybe back it out to be only for Chrome.

@TrySound
Copy link
Collaborator Author

TrySound commented Jan 13, 2018

Hm.. at first it looked good. I added WIP to not merge this PR yet :)

@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

Haha, no worries at all. This was my own rushed fault 😄

@mpospelov
Copy link

Hi guys, I'm not sure if I'm writing in proper place, but seems this PR brokes SSR on latest version

ReferenceError: window is not defined
    at isChrome (/Users/mikhailpo/rails_projects/salony-webapp-react/buildServer/webpack:/node_modules/react-virtualized/dist/commonjs/Grid/utils/maxElementSize.js:10:1)
    at getMaxElementSize (/Users/mikhailpo/rails_projects/salony-webapp-react/buildServer/webpack:/node_modules/react-virtualized/dist/commonjs/Grid/utils/maxElementSize.js:14:1)
    at new ScalingCellSizeAndPositionManager (/Users/mikhailpo/rails_projects/salony-webapp-react/buildServer/webpack:/node_modules/react-virtualized/dist/commonjs/Grid/utils/ScalingCellSizeAndPositionManager.js:47:1)

@bvaughn
Copy link
Owner

bvaughn commented Jan 16, 2018

Yikes. Yeah, that looks like. All window references should be guarded behind a typeof window !== "undefined" check. @TrySound or @mpospelov would you be willing to submit a PR to fix this? I'm very busy today.

@TrySound
Copy link
Collaborator Author

Yep #970

@TrySound
Copy link
Collaborator Author

@mpospelov Fixed in 9.17.3

@bvaughn
Copy link
Owner

bvaughn commented Jan 16, 2018

Woo hoo! Thanks buddy.

@mpospelov
Copy link

@TrySound Thanks a lot 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants