-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Add 'Read More' web component #11666
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
base: master
Are you sure you want to change the base?
feat: Add 'Read More' web component #11666
Conversation
…e and package.json; update package-lock.json to include lit dependencies.
- Introduced a new web component, OLReadMore, that supports height-based and line-based truncation. - Updated index.js to import the new component for registration as a custom element.
- Included the new OL components script in the footer for enhanced functionality. - Replaced the existing book description implementation with the OLReadMore component for better user experience with expandable/collapsible content.
- Changed 'Read more' and 'Read less' to 'Read More' and 'Read Less' for uniformity in capitalization. - Removed unnecessary margin-left style from the chevron icon.
- Changed 'Read more' and 'Read less' to 'Read More' and 'Read Less' for uniformity in capitalization in the book description section.
for more information, see https://pre-commit.ci
…oved accessibility and functionality - Replaced button elements with <details> and <summary> for better semantic structure. - Updated styles for toggle behavior and chevron icon. - Simplified the logic for handling content expansion and truncation. - Enhanced user experience by managing scroll behavior when collapsing content.
…library into feat/web-component-read-more
|
@lokesh I didn't test this one but just looking how the code structure I feel it's a pretty nice improvement! |
|
@lokesh great work on this one. I'm starting here with a functional review and then will review the code shortly after. To test this ssh into docker and run Testing Without a Read More ButtonI modified http://localhost:8080/works/OL61982W/Odyssey to have a short description so it doesn't have a read more button and it looks identical to how it looked before. Testing The Read More ButtonThis book already has enough text for a read more button: VideosBeforeBuilt-in.Retina.Display.2026-01-06.13.42.43.mp4AfterBuilt-in.Retina.Display.2026-01-06.13.40.47.mp4Expanded side by side:
Collapsed side by side:
Mobile
|
RayBB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think this is a really nice potential improvement. Great job @lokesh
I've left some feedback here.
However, I will note:
- Mek talked today a lot about prioritizing things. One could argue this PR is not very important to benefit the patrons. On the other hand, I'd argue it's a nice isolated starting point in a high visibility portion of the site. Getting the first piece out the door itself is important so that the more difficult ones can come later.
- I'm reviewing this based on the assumption that the staff are all onboard with the idea of doing these lit components. I don't remember if it was agreed with certainty while I was there but it certainly felt likely.
- A brief reminder that I am a volunteer. So while I've been here a while and can help staff by reviewing PRs and speed up the process a bit, sometimes, especially with new code like this, staff have quite different opinions on how to approach things. So staff might later ask that some things I suggested change and whatnot. Please be patient with that!
- Also, always feel free to push back on my suggestions as this is fairly new to me as well!
| components: | ||
| mkdir -p $(BUILD)/components_new | ||
| BUILD_DIR=$(BUILD)/components_new npx vite build -c openlibrary/components/vite.config.mjs | ||
| mkdir -p $(BUILD)/components | ||
| rm -rf $(BUILD)/components | ||
| mv $(BUILD)/components_new $(BUILD)/components | ||
|
|
||
| lit-components: | ||
| mkdir -p $(BUILD)/lit-components_new | ||
| BUILD_DIR=$(BUILD)/lit-components_new NODE_ENV=production npx vite build -c openlibrary/components/lit-vite.config.js | ||
| mkdir -p $(BUILD)/lit-components | ||
| rm -rf $(BUILD)/lit-components | ||
| mv $(BUILD)/lit-components_new $(BUILD)/lit-components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm of the opinion (and staff might see it differently) that we should do this:
lit-components(new)vue-components(what is currently components)components(runs the two above)
This way, it's one less new thing for people to figure out and keep in mind. Also the naming makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lit-components (new)
vue-components (what is currently components)
components (runs the two above)
Your breakdown makes sense to me. I kept the components rule as-is in my PR to keep the impact to a minimum as this would entail a couple of additional doc updates, but I might have been overly cautious.
| @@ -0,0 +1,54 @@ | |||
| /* eslint-env node, es6 */ | |||
| /** | |||
| * Vite configuration for building Lit web components | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be .mjs like https://github.com/internetarchive/openlibrary/blob/master/openlibrary/components/vite.config.mjs is ?
I think that it was recommended back when I first setup Vite but not entirely sure if it actually benefits us but we should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, I think this is a good change. I'll use the mjs extension.
| ?open="${this._expanded}" | ||
| @click="${this._handleToggle}" | ||
| > | ||
| <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally created the issue suggesting to use summary/details for this because I saw it mentioned somewhere.
Unfortunately, as AI as kindly shared with me now, it's:
Misuse of <details> and <summary>
You are using the <details> element but putting all the content inside the <summary> tag.
The Issue: Semantically, <summary> is a label/legend. Putting paragraphs of text inside it creates an invalid accessibility tree. Screen readers treat <summary> like a button; they don't expect a document inside it.
The Hack: You are fighting the browser’s native behavior with e.preventDefault() to keep the "summary" open while simulating a state change.
We might want to use a div + button or something similar that's accessible. This area isn't my strong suit.
It might also simplify the component JS a bit more?
| </div> | ||
| <ol-read-more max-lines="4" class="book-description" more-text="$_('Read More')" less-text="$_('Read Less')"> | ||
| $:sanitize(format(overview_desc)) | ||
| </ol-read-more> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are three total (two repeat) places we use the old readmore. Perhaps, we should set this up for all of those places so we can ensure it works consistently AND then in this very same PR we could delete all the old readmore code, that would make a nice show of the diff between old a new new then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create a separate PR that updates the other instances of the ReadMore macro with the new component and removes the old code. I don't think there is much harm in chunking the work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changed the Vite configuration file reference in the Makefile and package.json from `lit-vite.config.js` to `vite-lit.config.mjs`. - Removed the obsolete `lit-vite.config.js` file. - Updated the new Vite configuration file to include a comment indicating its purpose for Vue components.
…ling - Replaced <details> and <summary> elements with button elements for improved control over expansion and collapse. - Updated CSS class names for clarity and consistency. - Simplified event handling for toggling content visibility. - Enhanced button styles and added hover effects for better user interaction.
…ttribute to chevron icon - Updated the chevron SVG icon in the OLReadMore component to include the aria-hidden attribute for improved screen reader compatibility.
RayBB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it another test to check the changes and I think it's fantastic.
As far as I'm concerned this is fully functional and works very well.
There's a separate PR to do the rest of readmore and rip out the old code.
I'd still really like to get a review from @cdrini or someone else on the team since this it totally new lands for us.
PS: I'll be away for the next ~10 days so I won't be able to review or follow up on things much in that time. Hopefully it's merged in by the time I'm back!






feature: Add OLReadMore Lit web component for expandable/collapsible text
Technical
<ol-read-more>in openlibrary/components/lit/OLReadMore.jsopenlibrary/components/lit/index.jsedition/view.htmlandwork/view.htmltemplates to use the new component for main book descriptionsTesting
Screenshot
Stakeholders