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

Animate status bar max-height and margin-top #2981

Merged
merged 6 commits into from Jan 23, 2017

Conversation

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Jan 18, 2017

When collapsed, the max-height is set to 0px. When expanded, max-height is set to 50px, margin-top is set to 0px. When expanded and when the timeline is not scrolled down to the bottom, margin-top is set to -50px to offset the change in height, keeping it at the same scroll position.

Without the animation, there would be a jump when the user starts scrolling up from the bottom whilst the StatusBar is expanded.

Complements matrix-org/matrix-react-sdk#615

When collapsed, the max-height is set to 0px. When expanded, max-height is set to 50px, margin-top is set to 0px. When expanded and when the timeline is not scrolled down to the bottom, margin-top is set to -50px to offset the change in height, keeping it at the same scroll position.

Without the animation, there would be a jump when the user starts scrolling up from the bottom whilst the StatusBar is expanded.
@lukebarnard1

This comment has been minimized.

Copy link
Contributor Author

lukebarnard1 commented Jan 18, 2017

I'm wondering whether we should animate adding event tiles to the timeline itself, or at least the sticky scroll position so that the timeline doesn't suddenly jump.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Jan 18, 2017

(optionally) animating addition of roomtiles too would be lovely eyecandy, although i'd do it as a separate PR.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Jan 18, 2017

this is almost perfect :D

Slightly weird things:

  • The animation should probably be a bit more snappy - 200ms tops rather than 500ms
  • If you're scrolled up, and someone's typing, when you scroll down to the bottom you get a weird animation transition as the bar shifts into the "stuck to bottom" mode, which makes the scrolling feel quite strange - as if there's inertia on the scroll. It's a bit of an edge case, but i wonder if we can do better...
@@ -202,6 +202,25 @@ hr.mx_RoomView_myReadMarker {
width: 100%;
-webkit-flex: 0 0 auto;
flex: 0 0 auto;

max-height: 0px;
background-color: #fff;

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Member

please use $primary-bg-color or whatever the variable is in our brave new SCSS world

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Jan 19, 2017

Author Contributor

Is there something I might have to merge in to get that to work? I've got npm start:css running, but it doesn't seem to be doing SCSS

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Jan 20, 2017

Author Contributor

I think this might have to be done as a separate PR because this was branched prior to scss stuff.

z-index: 1000;
overflow: hidden;

-webkit-transition: all .5s ease-in-out;

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Member

as per main comment, suggest .2s

}

.mx_RoomView_statusArea_expanded.mx_RoomView_statusArea_mid_timeline {
margin-top: -50px;

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Member

hm. this is almost a beautiful hack, but it does result in a weird "inertial scrolling transition when you scroll from middle to bottom of timeline whilst the statusbar is visible. Feels like there should be a way to skip the transition in that scenario.

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Jan 19, 2017

Author Contributor

Using ease-out instead of ease-in-out on .mx_RoomView_statusArea seems to make this weird inertia feel less weird. Ideally we would do the transition based on the scroll position of the scroll view... or lose the _mid_timeline class earlier on.

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Jan 19, 2017

Author Contributor

or lose the _mid_timeline class earlier on.

would require some major threading-through of a theoretical isNearlyAtBottom on ScrollPanel of react sdk

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Jan 20, 2017

@lukebarnard1: is this meant to be back with me?

I was assuming that a possible correct fix for the weird scrolling would be to always have the status bar position: fixed at the bottom of the page, but animate the gap at the bottom of the screen required to house it in sync with the height of the element. So, if the statusbar is visible, no animations would be happening at all, so no scope for weird transitions when scrolling up & down the timeline?

@lukebarnard1

This comment has been minimized.

Copy link
Contributor Author

lukebarnard1 commented Jan 20, 2017

@ara4n If we animate the gap at the bottom of the screen, then we change the position of events in the timeline. This is fine when we're scrolled down to the bottom but not when we're mid-timeline. So however we manage it, we have to treat being mid-timeline differently.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Jan 20, 2017

From the files of police squad #riot-dev:

i got a solution ftr; will backlog it here for the record; no need to interrupt weekend tho
i think the correct solution is that rather than doing the negative margin-top hack on the StatusBar
you should ideally be scrolling the offset of the timeline when needed in order to compensate for the animation
so: if you are scrolled up on the timeline, and the statusbar is visible
then when the statusbar retracts, you would ideally compensate for it retracting by scrolling upwards by the same amount (as the animation progresses)
obviously this means syncing JS & CSS layout, which is pretty evil
one could make it better if the reveal/hide animation was done by JS (using velocity or whatever the post-velocity thing is that FB or someone came up with)
which should if anything be better perf than CSS anyway
so i think that'd be my recommendation in the long run
however, in the short term, i'd just ignore the problem and have the statusbar show/hide and always shift the timeline, whether at top or bottom.

@lukebarnard1

This comment has been minimized.

Copy link
Contributor Author

lukebarnard1 commented Jan 23, 2017

OK, so for now we shall

ignore the problem and have the statusbar show/hide and always shift the timeline, whether at top or bottom.

This will mean removing the CSS to change margin-top

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Jan 23, 2017

lgtm!

@lukebarnard1 lukebarnard1 merged commit ea82b90 into develop Jan 23, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@lukebarnard1 lukebarnard1 deleted the luke/feature-animated-status-bar branch Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.