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

fix #251 (finally) #596

Merged
merged 2 commits into from Apr 3, 2018

Conversation

Projects
None yet
2 participants
@atomiks
Copy link
Contributor

commented Mar 30, 2018

#251 has been left open too long I think, and this is the solution that @kybishop suggested, and the one I use personally, but it should be placed in update() so it works for resizing too.

Yes, it accesses the DOM and will reduce performance, but I don't think there's another way. Please test the performance difference before/after to see.

Object.assign() is being used, which I believe is already being ponyfilled in the lib.

@FezVrasta

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2018

Thanks! I'm going to take a look at this soon. How's the end result? No weird visual glitches or jumps?

@atomiks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2018

Looks like Object.assign isn't being transformed, probably need to add that babel plugin...

Test it here: https://codepen.io/anon/pen/Yavaav?editors=1010

All: can't see a "jump"/glitch where you can see the element redrawn at the top/left
Chrome: perfect when resizing (stays completely still)
Safari: slight 1px judders of the popper when resizing, but otherwise fine
Firefox: Horizontal scrollbars produced while resizing fast (possibly because of the way FF is debouncing resizing events?) but it ends up never overflowing once finished

@FezVrasta

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2018

I see that some tests are failing even on Chrome and Firefox, which do support Object.assign natively.

Anyway maybe we better get rid of it and use basic assignment?

@atomiks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

Updated it.

I also updated the CodePen so you can scroll and see how the updates work there as well. (seems to be fine). I'm going to test IE11 in a second.

Edit: No glitches while scrolling on IE11! 🎉 . I believe no browsers have glitches because the UI isn't redrawn until the JS has finished executing, which by that time the new Popper position styles have already been applied.

@atomiks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

  ✖ should update the tooltip position when changing the title
    IE 10.0.0 (Windows 8 0.0.0)
    IE 11.0.0 (Windows 10 0.0.0)

Looks like this test is failing for some reason...

@FezVrasta

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2018

Yup that test is a bit flaky, it's enough to run the plan again usually. Everything seems to work fine now!

Should I merge or do you need to do anything else?

@atomiks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

Did you measure the perf impact of it yet?

You mentioned that 1.14.0 updates in 0.3ms instead of 1ms, what is it with this change?

@FezVrasta

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2018

It still looks smooth
image

@FezVrasta FezVrasta merged commit 7bfdedf into FezVrasta:master Apr 3, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@atomiks atomiks deleted the atomiks:fix-251 branch Apr 3, 2018

FezVrasta added a commit that referenced this pull request Apr 4, 2018

FezVrasta added a commit that referenced this pull request Apr 4, 2018

FezVrasta added a commit that referenced this pull request Apr 4, 2018

FezVrasta added a commit that referenced this pull request Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.