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

Fix: Video description collapsible and no longer hijacks page scroll #5665

Open
wants to merge 18 commits into
base: development
Choose a base branch
from

Conversation

sabs21
Copy link

@sabs21 sabs21 commented Sep 8, 2024

Fix: Video description collapsible and no longer hijacks page scroll

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #4821

Description

Long video descriptions will now have their overflow hidden to prevent hijacking the scroll of the page. To see the full description and reintroduce the scrollbar, the user must click 'Click to View Description' shown at the bottom of the description. The 'Click to View Description' button is only shown for descriptions with a computed CSS height greater than or equal to 300 pixels.

Screenshots

CURRENT:
image

CHANGE (Description not expanded):
image

CHANGE (Description expanded):
image

Testing

I tested this change on a video with a long description (description exceeds CSS height of 300px) and a video with a short description. I verified:

  • Clicking 'Click to View Description' makes the description scrollable and that the button removes itself.
  • Short descriptions do not show the 'Click to View Description' button.

Desktop

  • OS: Windows 10
  • OS Version: 22H2, Build 19045.4842
  • FreeTube version: v0.21.3 Beta

Additional context

introduced a button in video descriptions shwon as 'Click to View Description'
which allows the user to see the full description and scroll within
the description card.
description length is based on computed css height of description
container
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 8, 2024 01:31
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 8, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc

Could you explain why you chose for 300px? Is the behavior the same as YT? If not why is this better?

@sabs21
Copy link
Author

sabs21 commented Sep 9, 2024

Could you explain why you chose for 300px? Is the behavior the same as YT? If not why is this better?

I chose 300px because that's the height/max-block-size the description was already set to (look at .videoDescription in WatchVideoDescription.css).

@kommunarr
Copy link
Collaborator

kommunarr commented Sep 16, 2024

My (purely visual, untested) feedback: I like YT's UI for this better, particularly that the click area is the full description box so it's easier to click, the "more" button is casual, placed inside the flow of the description, and not too strongly competing for visual attention with an underline (and at least for En-US users, in lower case), and the corresponding "less". Also like that it does so by fully unraveling it rather than just making the div scrollable.

@sabs21
Copy link
Author

sabs21 commented Sep 16, 2024

My (purely visual, untested) feedback: I like YT's UI for this better, particularly that the click area is the full description box so it's easier to click, the "more" button is casual, placed inside the flow of the description, and not too strongly competing for visual attention with an underline (and at least for En-US users, in lower case), and the corresponding "less". Also like that it does so by fully unraveling it rather than just making the div scrollable.

Agreed. I went ahead and implemented your feedback in another branch. Here are screenshots of what I did. If this looks like its on a better track, I'll merge my new branch into this one.

The only piece here that I didn't do is make the description fully unravel. The reason is that for very long descriptions, the description becomes longer than the recommended videos component on the right. This causes the page to auto load recommended videos which makes the page longer, causing the description to partially scroll back up and makes scrolling to the bottom of the description difficult.

On load:
image

Hovering over the description (entire description is clickable while collapsed):
image

Clicked description:
image

Show less:
image

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Sep 23, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@efb4f5ff-1298-471a-8973-3d47447115dc

@kommunarr any additional thought on the way its implemented now?

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 28, 2024
@kommunarr
Copy link
Collaborator

Sorry for the delay, that looks great @sabs21! Please merge that in.

auto-merge was automatically disabled October 4, 2024 00:07

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 4, 2024 00:07
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

Discovered one more issue; the "Show more" button is at the bottom of the tab order, so if a video description has links, you will have to tab through all of them to actually click the "Show more" button. It should instead be at the top of the tab order for the container. One possible solution is to use the CSS order property. Here's a video with links in the description as an example.

@kommunarr kommunarr added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 27, 2024
auto-merge was automatically disabled October 29, 2024 02:30

Head branch was pushed to by a user without write access

@sabs21
Copy link
Author

sabs21 commented Oct 29, 2024

Discovered one more issue; the "Show more" button is at the bottom of the tab order, so if a video description has links, you will have to tab through all of them to actually click the "Show more" button. It should instead be at the top of the tab order for the container. One possible solution is to use the CSS order property. Here's a video with links in the description as an example.

Great idea, I wound up flipping the ordering such that the buttons came first in the HTML, then used flexbox + order to adjust physical placement while keeping the desired tab ordering.

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 29, 2024 02:30
@kommunarr
Copy link
Collaborator

kommunarr commented Oct 29, 2024

For other testers, here's a video with a short description, where it properly does not show the "Show more" button.

issue (pre-existing, non-blocking): seeing template errors for videos with no description (ex) due to no use of the null access operator.

Screenshot_20241028_225247

issue: Sorry for all the different rounds of feedback, but I think this should be the last! I don't mind the "Show less" button being at the top, but it should have the same top padding as the p.description that it replaces.

auto-merge was automatically disabled October 30, 2024 00:55

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 30, 2024 00:55
@sabs21
Copy link
Author

sabs21 commented Oct 30, 2024

I went ahead and changed the visual ordering of the show less button so that the padding is normal once again. I didn't notice that the show less button was ordered differently due to me messing around with CSS locally in the dev tools. The spacing now appears normal once again too.

image

@kommunarr
Copy link
Collaborator

As a byproduct of this change, clicking the Show more button now opens at the end of the container (i.e., scrolled all the way down) instead of at the top.

auto-merge was automatically disabled October 30, 2024 22:33

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 30, 2024 22:33
@sabs21
Copy link
Author

sabs21 commented Oct 30, 2024

As a byproduct of this change, clicking the Show more button now opens at the end of the container (i.e., scrolled all the way down) instead of at the top.

I reverted the previous change and just added the margin to the top of the show less button

@kommunarr kommunarr added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Nov 23, 2024
@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Nov 24, 2024

Anyone got a video with short description for testing?

Update 1: The one in one of the comments is removed

@PikachuEXE
Copy link
Collaborator

Just got error when trying to find a video with short description
https://youtu.be/ASeJV9KGAOc

image

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Make the description box collapsible
5 participants