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: Use CSS transition to make room for composer #16750

Merged
merged 5 commits into from May 17, 2022

Conversation

nbianca
Copy link
Member

@nbianca nbianca commented May 13, 2022

The composer is displayed over the bottom part of the page. To make sure
that no content is covered by the composer, a bottom padding is added
equal to the height of the composer. When the composer is opened or
closed that padding is added after around 300ms because of a debounce.

This commit makes sure that the padding is added as soon as the composer
state changes and uses a CSS transition for a smooth user interface.

Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

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

Hmm, the padding bottom value will still be set by the debounced move function, after 300ms, so the values set here probably don't take effect.

Maybe we can try just adding the CSS transition rule and see if that fixes the issue? It probably does the needed job, given that #main-outlet is already in the DOM.

@nbianca
Copy link
Member Author

nbianca commented May 13, 2022

The problem is that the bottom padding value that is set by debounceMove is set too late and that causes the page to bounce a little. The CSS transition rule cannot help if the padding bottom is set too late. The issue is more visible when the composer is closed and the padding must be reset. The transition is just a trick to smoothen to user interface.

@pmusaraj
Copy link
Contributor

I see, thanks for clarifying.

So maybe we limit this change to when the composer is closing? I.e. immediately reset padding bottom when closing, and let the debounce function do its thing when opening. I feel a bit uneasy about setting it first to 300px and then to 400 (or whatever the height of the composer is).

The composer is displayed over the bottom part of the page. To make sure
that no content is covered by the composer, a bottom padding is added
equal to the height of the composer. When the composer is opened or
closed that padding is added after around 300ms because of a debounce.

This commit makes sure that the padding is added as soon as the composer
state changes and uses a CSS transition for a smooth user interface.
@nbianca nbianca force-pushed the fix_composer_padding_bottom branch from 0ffd0ab to f7d638f Compare May 13, 2022 20:49
@nbianca
Copy link
Member Author

nbianca commented May 13, 2022

I think it actually helps when opening the composer too. For example, try to open a composer and quickly scroll to the bottom. You will not be able immediately after opening the composer (first second), but then you can. That's because the padding is set after a while. It's not just 300ms, it's actually time_to_render + 300ms.

I do not find it a problem if it is set first to 300px and then to 400px. The transition CSS rule will smoothen that change anyway and the value of "300px" is just an intermediary value on the path from "0px" to "400px" or whatever the target value is. It would be a problem only if the value was larger than the target one (then it would be "0x" -> "300px" -> "target value less than 300px").

@pmusaraj
Copy link
Contributor

It would be a problem only if the value was larger than the target one (then it would be "0x" -> "300px" -> "target value less than 300px").

This can happen, though. For example, I open the composer, resize it to a small size, hit Cancel. Then on reopening, it remembers my resize, so it will reopen there.

Not entirely against trying out the intermediate 300px step, but it feels like it has potential to still be jumpy on various window sizes/devices.

@nbianca
Copy link
Member Author

nbianca commented May 16, 2022

I found a workaround for that issue. After you resize the composer, the height property sticks between composer open+close, so we can use the height property if it exists or fallback to the default value of 300px.

I am currently looking at the CSS solution. I found a possible problem, we set a min-height property on the composer, which will interact with that value, but I will see what I can do about it.

@nbianca
Copy link
Member Author

nbianca commented May 16, 2022

I think a CSS only solution would work, but there are a few edge cases we need to cover. For example, now we use offsetHeight to return the "real" height for the composer, including the padding and margin. If we move to CSS, we will have to compute that and be careful when overwriting the styles.

The last issue is that we have different sizes for composer depending if we edit the title or not. I assume with a bit of hacking, that is possible too.

I just pushed a CSS properties based solution.

Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

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

Thanks!

@nbianca nbianca merged commit 9ea8a4a into main May 17, 2022
@nbianca nbianca deleted the fix_composer_padding_bottom branch May 17, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants