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

ToC show/hide animation is gone, but the delay is still there #686

Closed
AlexDaniel opened this issue Jul 10, 2016 · 14 comments
Closed

ToC show/hide animation is gone, but the delay is still there #686

AlexDaniel opened this issue Jul 10, 2016 · 14 comments

Comments

@AlexDaniel
Copy link
Member

[show]/[hide] buttons used to trigger a nice css animation, but it is no longer there.

However, the animation delay is still there. That is, you press [hide], then wait 1 second, and only then it actually hides.

Altai-man added a commit that referenced this issue Jul 10, 2016
@Altai-man
Copy link
Member

Altai-man commented Jul 10, 2016

Will #687 fix the problem? If yes, merge it please.

@AlexDaniel
Copy link
Member Author

If I'm not mistaken, your pull request removes the animation completely. However, another fix would be to fix the animation.

Do we want the animation to be there or not?

@gfldex
Copy link
Contributor

gfldex commented Jul 10, 2016

CSS animations are fine in my eyes because the reader can make them go away with a userstyle. The less javascript we got the better. A botched setInterval can drain a battery rather quickly.

@Altai-man
Copy link
Member

Yes, you are right. I can revert js animation, firstly I thought title somewhat implies that delay should be removed.
If you want CSS animation as a substitution of js version, we can remove js version for now and create a ticket like "Implement css animation for ToC box". What do you think?
As for me, animation is unnecessary, but it doesn't bother me in any way if it's quick enough.

@gfldex
Copy link
Contributor

gfldex commented Jul 10, 2016

please remove the JS version

@zoffixznet
Copy link
Contributor

zoffixznet commented Jul 10, 2016

The show/hide feature was originally added as an easier way to get to content. Now that gfldex++ moved the ToC to the side, there's absolutely no reason for the existence of the show/hide feature and it should be yanked out entirely.

This is the commit that added it. 4932a10

Note that removing it would involve tossing out the cookie plugin, its mention the licensing section in readme, and the notice in the footer about using cookies.

@AlexDaniel
Copy link
Member Author

moved the ToC to the side, there's absolutely no reason for the existence of the show/hide feature and it should be yanked out entirely

And now I realized that it looks completely different on larger screens…

On a small screen the toc is still on the top, so [show]/[hide] feature still makes sense (or not?). Also, the links are blue, hmm…

@zoffixznet
Copy link
Contributor

zoffixznet commented Jul 10, 2016

On a small screen the toc is still on the top, so [show]/[hide] feature still makes sense (or not?).

Oh, right, I forgot about smaller screens. I guess there is still use for it. The animation can likely be fixed by changing ol to tbody in here and here though I think there was a discussion above about removing the JS version.

@Altai-man
Copy link
Member

Now this situation looks more interesting.
So we have two choices:
1.Fix JS animation.
2.Remove JS animation and implement CSS animation(without state storing in the cookies) instead later.
I suppose, we can't throw away show/hide feature completely, because of small screens.
I can discard my pr or extend it to full removing of js part, but, I afraid, I can't make a css implementation, so new issue still will be created. And I can't decide what to choose by myself, since I'm quite a novice here.

@Altai-man
Copy link
Member

Closed by #767

@Altai-man
Copy link
Member

It seems we should reopen this.

Current css animation doesn't affect height and based on font-size/padding/margin transformations. Due to this, element sizes are still the same. even if element is not visible. It's not really bad, but when someone uses a small screen, ToC moves to the page center and we have an empty space. It's bad.

The problem is: to animate show/hide, we need to know exact max-height of element(auto doesn't work). And this height is different for every page. Possible solutions are:

  • Add some js to compute max-height and inject it into css every time(i.e. js+css variant, maybe it will be more efficient than js version).
  • Throw away css animation and use js-animation.
  • Throw away animation at all.

@Altai-man Altai-man reopened this Jul 27, 2016
@gfldex
Copy link
Contributor

gfldex commented Jul 27, 2016

Animations on web pages have two functions. Convey time based information to the viewer simply be using progression of time. Guide the attention of the viewer to changed content.

There is no time based information to be conveyed. If you miss the fact that the ToC just disappeared, no amount of animation will help you. Also, we don't got viewers, we got readers. Reading and animations don't mix.

I'm a big fan of form follows function. If you can show me the function of that animation I'm happy to fix it.

@Altai-man
Copy link
Member

Altai-man commented Jul 27, 2016

I suppose, we should switch to js-animation for now, since it's bugless(or better than current css-solution anyway) and open issue like "Create a cool and free and better css-animation" in case if some css-priest/magician(or just someone better than me) will be here looking for a work.
UPD: or just throw away animations, of course.

@Altai-man
Copy link
Member

Finally closed with d0dfc5a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants