Skip to content

Fix and simplify resize handling using ResizeObserver #3411

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

Merged
merged 3 commits into from
Jan 21, 2019

Conversation

igorsdv
Copy link
Contributor

@igorsdv igorsdv commented Jan 12, 2019

Fixes #3227.

Terminals do not get properly resized when switching between single-tab and multiple-tab layout, causing a rendering issue at the bottom of the viewport. This can also be triggered when splitting panes vertically in a small window.

This is fixed by using a ResizeObserver on the wrapper element to trigger the resize, which incidentally simplifies a lot of the resizing code. In particular, onOpen is no longer needed as the observer fires immediately when registered.

Note that ResizeObserver is an experimental API, but has been available on Chrome stable since Chrome 64.

Copy link
Contributor

@juancampa juancampa left a comment

Choose a reason for hiding this comment

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

Nice simplification! The root cause of the issue is that we're animating the term-group's margin-top but only resizing once (at the beginning, when the div is biggest)

I'd like to include this in the next canary release but we also need to disable said animation. Otherwise many resizes happen in quick succession giving us some nice jank (resize is costly)

Do you mind removing this line in your PR as well?https://github.com/zeit/hyper/blob/canary/lib/components/terms.js#L148

@juancampa
Copy link
Contributor

juancampa commented Jan 18, 2019

Alternatively, we could keep the animation but change it to a transform: translateY(...) instead. Probably using animation instead of transition

@chabou
Copy link
Contributor

chabou commented Jan 18, 2019

Awesome work @igorsdv ! Thank you for your help 🙏
I'll try to review this ASAP.

I've always been frustrated by all these fit resizes.
Your solution seems really elegant.

@rauchg
Copy link
Member

rauchg commented Jan 18, 2019

That's amazing @igorsdv

@chabou
Copy link
Contributor

chabou commented Jan 18, 2019

I have a minor regression.

Terms are blinking when resize terms
hyper-resize

@juancampa
Copy link
Contributor

Hey @igorsdv, fyi I'm pushing the couple of changes directly to this PR, because we're trying to get it merged as soon as possible (It helps a lot :)

}

.terms_termsShifted {
margin-top: 68px;
animation: shift-down 0.2s ease-out;
Copy link
Contributor

Choose a reason for hiding this comment

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

I made the shift-down a little faster (and eased out) because it overlaps a bit with the tabs

@igorsdv
Copy link
Contributor Author

igorsdv commented Jan 21, 2019

Thank you @onecamp! Sorry I didn't get around to making the changes over the weekend!

@chabou chabou merged commit ea3e109 into vercel:canary Jan 21, 2019
@juancampa
Copy link
Contributor

Thank you @onecamp! Sorry I didn't get around to making the changes over the weekend!

No worries, this was super helpful!

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.

Terminal prompt is beyond viewing range
4 participants