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.

Sign up for GitHub

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

Jump to bottom

Clean animatedScaleComponent and decide consistently whether to show unread notice or not. #3027

Closed
wants to merge 3 commits into from
Closed

Clean animatedScaleComponent and decide consistently whether to show unread notice or not. #3027

wants to merge 3 commits into from

Conversation

jainkuniya
Copy link
Member

No description provided.

@jainkuniya jainkuniya force-pushed the improve-unread-notice branch from be87674 to 97a0624 Compare October 9, 2018 19:02
jainkuniya
jainkuniya commented Oct 9, 2018
View reviewed changes
Copy link
Member Author

@jainkuniya jainkuniya left a comment

Choose a reason for hiding this comment

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

@borisyankov Updated!

@jainkuniya jainkuniya mentioned this pull request Oct 13, 2018
Open
@jainkuniya jainkuniya force-pushed the improve-unread-notice branch from 97a0624 to 72b8be0 Compare November 20, 2018 08:40
@borisyankov
Copy link
Contributor

The code looks reasonably correct at first glance. Good job!
I would test the commit manually later to see if it works in practice.

What needs a bit more work here are the commit messages.
They are not easy to understand.
Please, go and read them carefully and think about how you can improve them. 👍

@jainkuniya jainkuniya force-pushed the improve-unread-notice branch from 72b8be0 to dd1f35a Compare November 29, 2018 11:55
jainkuniya
jainkuniya commented Nov 29, 2018
View reviewed changes
Copy link
Member Author

@jainkuniya jainkuniya left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@borisyankov Updated! 👍

On my local, order of commits is like
image

Edit: Before rebasing, Github UI was showing different order.

@jainkuniya jainkuniya force-pushed the improve-unread-notice branch from dd1f35a to 322a9e7 Compare December 15, 2018 12:04
@jainkuniya jainkuniya mentioned this pull request Jan 24, 2019
Closed
@jainkuniya
animatedScaleComponent: Do not render children if not visible.
ab87aac 
If the component is not visible, then due to animated style it's scale
is 0 which is causing a sudden splash of the children for a moment. A
solution for such sudden splash was introduced in 0baca8f. Now using
the same logic make it generic so that it will be applicable to all
other components which are using `AnimatedScaleComponent`.

`AnimatedScaleComponent` is also used by `UnreadNotice`: In the
`AnimationScaleComponent` we have been taking help of `display`
property (which acts same as it does in CSS), for same purpose. So
when visible is false, we were making display none just after
animation ends. Thus making it's size 0.

Now when visible becomes false, we will not be rendering anything
inside animation component and size thus will be 0. Thus no need to
display propery.

Basically this is a alternative of using display property. So next
`display` can be removed.

It's advantage over that is, it will result in smooth transition.
Before when visible becomes false, it scale down but height/size
remains same (resulting in white space) and as soon as animations
get's over it change display to none thus resulting in a sudden change.
@jainkuniya
autocompleteView: Remove shouldShow condition from children rendering.
a4a69ec 
This was introduced in 0baca8f. In the previous commit this logic is
being handled in the `AnimatedScaleComponent` itself, so removing it
from here.
@jainkuniya
animatedScaleComponent: Remove use of display property.
9b45beb 
It works same as it works in CSS. There is no use of such
property in animation component, as they result in a sudden change.

It was used to make size 0 whenever component is not supposed to show.
But if there is nothing to show then it's size will be automatically
0.

This will not cause any issue. It has been used in unread notice and
autocomplete popup. Refer previous commit, which is a better
alternative for this in unread notice.

For autocomplete, when it is not suppossed to pop, it's children will
be null and nothing will be rendered. Thus taking no space (refer
0baca8f). Part of zulip#2943.
@jainkuniya jainkuniya force-pushed the improve-unread-notice branch from 322a9e7 to 9b45beb Compare February 24, 2019 10:08
jainkuniya
jainkuniya commented Feb 24, 2019
View reviewed changes
Copy link
Member Author

@jainkuniya jainkuniya left a comment

Choose a reason for hiding this comment

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

@gnprice @borisyankov Let's review this too, so that autocomplete pop up animations starts working. :)

@gnprice
Copy link
Member

gnprice commented Feb 25, 2019

Thanks @jainkuniya !

Have you tried out this branch with each of the UIs it affects?

I took a quick look:

  • The FAB on the user picker (if you go to start a group PM conversation) now disappears abruptly, rather than shrink smoothly.

    • The smooth shrinking feels better to me. So perhaps this doesn't belong as a universal rule for this shared component.

  • Looking at UnreadNotice: the effect is that when the messages are read, the notice now just disappears abruptly with no animation.

    • That doesn't feel like quite what we want. It feels pretty abrupt.
    • The status quo, with the white space and then a jump, feels more wrong. So maybe this strategy can be an improvement there. But, see above, it probably shouldn't be universal.

  • Looking at the compose autocompleters, this makes it grow with an animation, but it disappears abruptly. Is that intentional? I'm not sure it's what we want.

(Curious also what thoughts @borisyankov has.)

@jainkuniya
Copy link
Member Author

Closing this, as this is now in stale state.

@jainkuniya jainkuniya closed this Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borisyankov borisyankov borisyankov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jainkuniya @borisyankov @gnprice