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
lokesh
changed the title
…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
RayBB
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
- 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
RayBB
left a comment
There was a problem hiding this comment.
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!
cdrini
left a comment
cdrini
left a comment
There was a problem hiding this comment.
Two pieces of feedback:
- During page load before the JS has come in, there's a flash where the content is untrimmed, causing reflow. We'll want to make it start at the minimal max height to avoid content shift which will ding our google core web vitals (and isn't great UX!)
- Restoring the animation when the expander closes/opens would be a nice UX touch
But not blockers ; this can be merged in!
* Enhance build process by adding support for lit-components in Makefile and package.json; update package-lock.json to include lit dependencies. * Add OLReadMore component for expandable/collapsible content - 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. * Add OL components and update book description rendering - 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. * Update OLReadMore component text for consistency - 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. * Update capitalization of OLReadMore component text for consistency - Changed 'Read more' and 'Read less' to 'Read More' and 'Read Less' for uniformity in capitalization in the book description section. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Refactor OLReadMore component to use <details> and <summary> for improved 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. * Update build configuration for Lit components - 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. * Refactor OLReadMore component to enhance toggle functionality and styling - 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. * Enhance accessibility of OLReadMore component by adding aria-hidden attribute to chevron icon - Updated the chevron SVG icon in the OLReadMore component to include the aria-hidden attribute for improved screen reader compatibility. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
4 participants
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