-
-
Notifications
You must be signed in to change notification settings - Fork 635
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 sidebar height & hide ScrollUpButton until scrolled (fade in) #384
Conversation
Fixes gotify#383 and issue found within gotify#276
Codecov Report
@@ Coverage Diff @@
## master #384 +/- ##
=======================================
Coverage 88.92% 88.92%
=======================================
Files 44 44
Lines 1436 1436
=======================================
Hits 1277 1277
Misses 85 85
Partials 74 74 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. This actually has been bugging me too.
I've added remarks in subcomments.
Feedback implemented @jmattheis |
ui/src/common/ScrollUpButton.tsx
Outdated
|
||
componentWillUnmount() { | ||
if (typeof window !== 'undefined') { | ||
window.removeEventListener('scroll', this.scrollHandler.bind(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this doesn't work as .bind will give a new reference and thus, doesn't unregister the old handler. I'll fix this. Also, I'll prevent some rerenders, by checking if the state will be changed.
cb8e3f7
to
29a0171
Compare
Fixes #383 and issue found within #276