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

Fixed Read More Button Display Logic to Avoid Unnecessary Display #9871

Closed
wants to merge 3 commits into from

Conversation

CodeMaverick2
Copy link
Contributor

@CodeMaverick2 CodeMaverick2 commented Sep 11, 2024

Closes #9329

Description

This PR addresses an issue where the "Read more" button was displayed even when it made the section longer than the fully expanded content. This led to a confusing user experience, as the "Read more" button was not serving its intended purpose. The following changes have been made to resolve this issue:

Changes

  1. Capture "Read More" Button Height:

    • The height of the "Read more" button is now captured during initialization to ensure accurate calculations.
  2. Update Condition in reset Method:

    • The logic that determines whether the "Read more" button should be displayed has been updated to include the height of the button. This ensures the button is only shown when it makes sense.

Impact

  • Improved User Experience: The "Read more" button is now only displayed when necessary, preventing unnecessary clutter and confusion.
  • Correct Layout Behavior: Ensures the layout behaves as expected, with the "Read more" button enhancing usability rather than detracting from it.

Testing

To test visit a link like https://testing.openlibrary.org/books/OL8978501M/Robin_Hood

Screenshots

Screenshot from 2024-07-25 18-43-42

@CodeMaverick2
Copy link
Contributor Author

@RayBB I have done the changes recommended by @cdrini

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.42%. Comparing base (ce16a79) to head (90a10f0).
Report is 399 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9871      +/-   ##
==========================================
+ Coverage   16.06%   16.42%   +0.36%     
==========================================
  Files          90       92       +2     
  Lines        4769     4907     +138     
  Branches      832      856      +24     
==========================================
+ Hits          766      806      +40     
- Misses       3480     3565      +85     
- Partials      523      536      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RayBB RayBB added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Sep 12, 2024
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

@CodeMaverick2 I pushed up a small change so that the tests pass and shouldn't impact functionality. Otherwise this looks good.

Unfortunately, my deploys to testing seem to be failing and I'm not sure why.
However, I've deployed it to gitpod and it's working for me.

Lets just get a final check from @cdrini

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Sep 12, 2024
@CodeMaverick2
Copy link
Contributor Author

@RayBB Sure, @cdrini Can u check the problem

@CodeMaverick2
Copy link
Contributor Author

@RayBB Are there any more changes that need to be done in this

@RayBB
Copy link
Collaborator

RayBB commented Sep 19, 2024

We are waiting for staff to review. They often do this on Monday. If not feel free to tag @cdrini again

@CodeMaverick2
Copy link
Contributor Author

@cdrini What's the status here ?

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

One small bug, otherwise looking great! 😊

@@ -55,7 +55,7 @@ export class ReadMoreComponent {
this.fullHeight = this.$content.scrollHeight;
// Fudge factor to account for non-significant read/more
// (e.g missing a bit of padding)
if (this.$content.scrollHeight <= (this.collapsedHeight + 1)) {
if (this.$content.scrollHeight <= (this.collapsedHeight + (this.$readMoreButton?.offsetHeight | 0) + 1)) {
Copy link
Collaborator

@cdrini cdrini Sep 23, 2024

Choose a reason for hiding this comment

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

Nice this is working almost perfectly ! A few tweaks:

  • We'll want to compute and store the offsetHeight of the readMore button the first time reset is called. Otherwise when the read more button is hidden and reset is called, it will switch between existing and not existing. (You can observe this by trying to resize the window on testing. You'll see the read more button appear and disappear, since sometimes the height is there and sometimes it's zero). The first time reset is called, the read more is always visible, so you can likely do something like: this.readMoreHeight = this.readMoreHeight || this.$readMoreButton.offsetHeight; at the top of this method.

  • Also we don't need the ?. since it's always defined, and we don't need to default to zero since offsetHeight will always be a valid number.

  • (also: Small typo, I think you mean || instead of | 😁 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out—I’ve just fixed it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need to implement that first bullet point as well!

@cdrini cdrini added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. labels Sep 23, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Sep 26, 2024
@CodeMaverick2
Copy link
Contributor Author

@RayBB @cdrini The tests are failing like previously not able to figure out why

@cdrini
Copy link
Collaborator

cdrini commented Sep 27, 2024

Nice! There's still the first point; we'll want to fetch the height value only once (see the first bullet point)

Oh the tests are failing cause it looks readMoreButton is null in the tests; You'll need to update the test so that it is not null 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Read more" button should not be added if it takes up as much space as the hidden text
4 participants