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

More transition effects #1117

Closed
wants to merge 1 commit into from
Closed

Conversation

bews
Copy link
Contributor

@bews bews commented Apr 29, 2017

This adds transition effects for almost every element with hover support.
Also transition + hover effects were added to a lot of buttons:
2017-04-30_02-15-39
2017-04-30_01-58-55
2017-04-30_01-58-18
2017-04-30_01-58-33
2017-04-30_01-58-46

In this menu background change covers the whole block now:
2017-04-30_02-02-11

Tooltipped animation-delay was decreased 0.4 -> 0.1 to match new transition effects.


TODO:

@bews bews mentioned this pull request May 3, 2017
3 tasks
@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label May 6, 2017
@bews bews force-pushed the bews/dev-3-transitions branch from a420008 to e65e97a Compare May 9, 2017 09:15
@bews bews force-pushed the bews/dev-3-transitions branch from e65e97a to bbb06f8 Compare May 11, 2017 04:40
Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

2 minor changes requested, but overall a nice detail-oriented PR, thanks!

@thelounge/maintainers, assuming @bews addresses my 2 comments, are you okay adding this in the 2.3.2 milestone?

-webkit-animation-delay: .4s;
animation-delay: .4s;
-webkit-animation-delay: .1s;
animation-delay: .1s;
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert this please? We actually re-added the animation delay some time ago on purpose to avoid fidgeting on timestamps. We'd be better off with default values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned before, it was done to match new transition effects. There are cases when these 2 animations happen simultaneously (settings, help, toggle user list, etc) , and having different animation duration makes them look bad.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I read your PR description 😅

However, your changes are not on an animation duration but on an animation delay. There is a reason we have a 0.4s delay on tooltips, it's to avoid the flickering when moving the mouse over by chance.

We need to keep this at .4s and I won't be able to 👍 until they're reverted. Would you mind doing so? 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can keep .4 for timestamps only.

Copy link
Member

Choose a reason for hiding this comment

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

No, we cannot 😅

We would really appreciate if you fix this so we can 👍 and merge this. Thanks! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we cannot

Why?

if you fix this

You're asking to break this, not fix.

Copy link
Member

Choose a reason for hiding this comment

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

Why?

To keep all our tooltips consistent, and this is not negotiable 😅

You're asking to break this, not fix.

Sigh
No, I'm not. How is keeping reasonable tooltip delays (again, not animation durations) breaking anything?
I may very well be missing something, but please explain me why it is needed to reduce the tooltip apparition delay? I honestly have no idea how the animation delay could collide with any other animation out there, but if it did, .1s would be more subject to do that than .4s.

Anyway, I really don't think there is anything to break here, so unless I'm missing something obvious, please change them back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.1:
1
.4:
2

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and again there are good reasons why we added a delay.

Anyway, I feel like we are going in circles here. This is my final review for this PR: please revert the animation delay change and you should get my 👍. Thanks!

color: #333;
margin-top: 6px;
margin-bottom: 6px;
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this has a tiny issue, in which it adds extra margin around elements within the same "group":

Before your PR After your PR
screen shot 2017-06-12 at 01 36 33 screen shot 2017-06-12 at 01 37 04

Mind fixing this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Um.. I didn't expect any groups here. This can be fixed only by returning to the old design. Are those groups a planned feature or I'm missing smth?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, the separator (divider in our HTML) is not meant to be present between all rows. The nearest and easiest addition to these dropdowns I can think of is having multiple user actions: kick, ban, etc. So rows would be: nick / divider / ban / kick / ...

@astorije
Copy link
Member

astorije commented Jul 8, 2017

@bews, are you okay reverting the animation delay change as requested in the review?

This PR is simple and 🎉 outside of that minor change needed, it should make it to master very quickly.

@bews
Copy link
Contributor Author

bews commented Jul 8, 2017

are you okay reverting the animation delay change

No, I'm not, it's a crucial thing here. Also, this PR is incomplete without #1130 (since I removed some parts from here), therefore it's not ready anymore.

Anyway, it might be better to leave these things to themes, so I close this.

@bews bews closed this Jul 8, 2017
@astorije
Copy link
Member

astorije commented Jul 8, 2017

That's frustrating, it was almost perfect (and some more time wasted...).

@bews
Copy link
Contributor Author

bews commented Jul 8, 2017

I wasted more time on these PRs than you, so things are not so bad 😸
All of this was supposed to be plugins from the beginning, but I was inspired by another project and made PRs instead. From now on I switch back to the plugins plan, so no wasting time anymore 😺

@astorije astorije mentioned this pull request Jul 9, 2017
@astorije
Copy link
Member

astorije commented Jul 9, 2017

The Good Parts of this has been ported to #1314. It literally took me 2 minutes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants