jwplayer / ott-web-app

Reference implementation for JWP-powered apps
Apache License 2.0
70 stars 52 forks source link

Fix / Restrict text length to avoid scrolling issue #538

Closed MelissaDTH closed 4 months ago

MelissaDTH commented 4 months ago

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.

Steps completed:

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

Example

github-actions[bot] commented 4 months ago

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

ChristiaanScheermeijer commented 4 months ago

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 commented 4 months ago
  • Update the TruncatedText component to just truncate the given text

I agree. I would go for option 1!

MelissaDTH commented 4 months ago

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!