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 / Restrict text length to avoid scrolling issue #538

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

MelissaDTH
Copy link
Contributor

Description

Issue: The video detail page starts halfway down the viewport when a media item has a very long description, because the "Start Watching" button receives focus.

Solution

By limiting the number of lines in the video description, we not only ensure that the page does not start halfway down, but it also aligns better with the designs and overall visuals.

  • I've introduced a reusable TruncatedText component and modified CollapsibleText to remove the unnecessary maxHeight prop. Added a maxHeight constant with the original value for readability in calculations.
  • Verified the fix with video descriptions of various lengths and tested with 175% zoom on smaller screens to ensure proper line limit and prevent page scrolling.

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

Example

Copy link

github-actions bot commented May 29, 2024

Visit the preview URL for this PR (updated for commit b7831b0):

https://ottwebapp--pr538-fix-scroll-detail-pa-z71kkux2.web.app

(expires Sat, 29 Jun 2024 16:19:55 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

@MelissaDTH MelissaDTH requested a review from langemike May 29, 2024 13:23
Copy link
Collaborator

@langemike langemike left a comment

Choose a reason for hiding this comment

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

Nice!

@ChristiaanScheermeijer
Copy link
Collaborator

Thanks for the updates @MelissaDTH! There is still one conceptual thing I didn't mention, and I'm curious if you and others agree.

I think we should simplify the TruncatedText component like the CollapsibleText component. Both have separate functions, but now the TruncatedText component is also responsible for choosing to truncate or collapse the text.

So I think we should:

  1. Update the TruncatedText component to just truncate the given text
  2. Either conditionally render either in the updated screens/components OR create a specific component that implements this logic

The advantage is that both components can be used agnostic and testing these components becomes more simple.

What do you think?

@langemike
Copy link
Collaborator

  • Update the TruncatedText component to just truncate the given text

I agree. I would go for option 1!

@MelissaDTH
Copy link
Contributor Author

I think we should simplify the TruncatedText component like the CollapsibleText component. Both have separate functions, but now the TruncatedText component is also responsible for choosing to truncate or collapse the text.
...

Thanks for the feedback, @ChristiaanScheermeijer. I spoke with Mike about this issue and during that conversation, I advocated for the idea that since TruncatedText doesn't have much functionality, it would simplify matters to assign it the responsibility of determining how to render the text based on whether it's being viewed on mobile or not. However, now that both of you suggest going in this direction, I've implemented it!

Copy link
Collaborator

@ChristiaanScheermeijer ChristiaanScheermeijer left a comment

Choose a reason for hiding this comment

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

Nice! 🎉

@MelissaDTH MelissaDTH merged commit 18a1990 into develop Jun 3, 2024
10 checks passed
@MelissaDTH MelissaDTH deleted the fix/scroll-detail-pages branch June 3, 2024 07:56
anoblet pushed a commit to conversionxl/ott-web-app that referenced this pull request Feb 17, 2025
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

Successfully merging this pull request may close these issues.

3 participants