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

redesign: misc. fixes for asciinema panes #532

Merged
merged 2 commits into from Sep 16, 2020

Conversation

@samueldr
Copy link
Member

samueldr commented Sep 16, 2020

We discussed about:

  • Pausing on close
  • Playing on open
  • Smooth scrolling

The first two are implemented. See the new custom event for that.

Smooth scrolling using el.scrollIntoView({ behavior: "smooth" }); does not smooth scroll, AFAICT.

So we would be left to choose an additional JavaScript library to handle the smooth scroll, only for the pane open/close sequence. We apparently can't rely on the browser to smooth scroll the site.

My personal experience with sites that scroll smoothly like that is that it's not ever needed, and most of the time feels wrong. For something I'm not entirely sure is needed.

I'll be leaving the decision for smooth scrolling to you, and I'll implement it if desired. Though it's back at the end of the queue for priorities for the time being. It would take a bunch of time to decide on the better library to use, time I prefer to use to implement the rest of the pages as MVPs.

@samueldr samueldr requested a review from garbas Sep 16, 2020
@samueldr samueldr added the re-design label Sep 16, 2020
Copy link
Member

davidak left a comment

It works to open the demo,
it starts playing and pauses when open again.
when starting another one and close it,
the first is still at the same time paused

@davidak
Copy link
Member

davidak commented Sep 16, 2020

I don't like that the background is just grey. It feels like it opens in a new tab and i'm not on the same page anymore.
The common and expected behavior would be to open it over the content and make a transparent layer over the content to darken it.

https://www.w3schools.com/bootstrap4/bootstrap_modal.asp

@garbas
garbas approved these changes Sep 16, 2020
Copy link
Contributor

garbas left a comment

Scrolling is still missing but the rest works as expected. I'll merge it - to be able to show it at the meeting - and we can improve the asciinema player in future PRs

@garbas garbas merged commit 17fd2cc into feature/2020-redesign Sep 16, 2020
2 checks passed
2 checks passed
build-and-deploy
Details
Netlify Netlify deployment
Details
@garbas garbas deleted the feature/2020-r/asciinema-fixes branch Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.