ActivityPub media cards UX improvement#22582
Conversation
ref AP-916, AP-915 - Audio and video cards cannot be played in the ActivityPub reader. This PR contains a temporary UX improvement that it renders audio and video cards in a wrapper that opens the original article in a new tab, so the cards don't look completely broken
WalkthroughThe pull request updates how article HTML content is processed and rendered within the ActivityPub admin module. A new parameter, Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/admin-x-activitypub/src/utils/content-formatters.tsOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-activitypub".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-activitypub/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/admin-x-activitypub/src/components/feed/ArticleModal.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-activitypub".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-activitypub/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
peterzimon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin-x-activitypub/src/utils/content-formatters.ts (1)
5-42: Consider refactoring to reduce code duplicationThe new
formatArticlefunction contains significant code duplication with the existingopenLinksInNewTabfunction. Both functions create a div, parse HTML, and modify links with the same attributes.Consider refactoring to extract the common functionality:
+export const processHtml = (content: string, processors: ((div: HTMLDivElement) => void)[]) => { + const div = document.createElement('div'); + div.innerHTML = content; + + for (const processor of processors) { + processor(div); + } + + return div.innerHTML; +}; + +export const addTargetBlankToLinks = (div: HTMLDivElement) => { + const links = div.getElementsByTagName('a'); + for (let i = 0; i < links.length; i++) { + links[i].setAttribute('target', '_blank'); + links[i].setAttribute('rel', 'noopener noreferrer'); + } +}; + +export const wrapMediaCards = (div: HTMLDivElement, postUrl: string) => { + if (!postUrl) return; + + const mediaCards = div.querySelectorAll('.kg-audio-card, .kg-video-card'); + for (let i = 0; i < mediaCards.length; i++) { + const mediaCard = mediaCards[i] as HTMLElement; + const wrapper = document.createElement('a'); + wrapper.href = postUrl; + wrapper.target = '_blank'; + wrapper.rel = 'noopener noreferrer'; + wrapper.style.cursor = 'pointer'; + wrapper.style.display = 'block'; + wrapper.style.textDecoration = 'none'; + wrapper.style.color = 'inherit'; + + mediaCard.parentNode?.insertBefore(wrapper, mediaCard); + wrapper.appendChild(mediaCard); + } +}; + export const formatArticle = (content: string, postUrl: string) => { - // Create a temporary div to parse the HTML - const div = document.createElement('div'); - div.innerHTML = content; - - if (postUrl) { - // Find all audio and video card divs - const mediaCards = div.querySelectorAll('.kg-audio-card, .kg-video-card'); - - // Wrap each media card in an anchor tag - for (let i = 0; i < mediaCards.length; i++) { - const mediaCard = mediaCards[i] as HTMLElement; - const wrapper = document.createElement('a'); - wrapper.href = postUrl; - wrapper.target = '_blank'; - wrapper.rel = 'noopener noreferrer'; - wrapper.style.cursor = 'pointer'; - wrapper.style.display = 'block'; - wrapper.style.textDecoration = 'none'; - wrapper.style.color = 'inherit'; - - // Move the media card into the wrapper - mediaCard.parentNode?.insertBefore(wrapper, mediaCard); - wrapper.appendChild(mediaCard); - } - } - - // Find all anchor tags - const links = div.getElementsByTagName('a'); - - // Add target="_blank" and rel attributes to each link - for (let i = 0; i < links.length; i++) { - links[i].setAttribute('target', '_blank'); - links[i].setAttribute('rel', 'noopener noreferrer'); - } - - return div.innerHTML; + return processHtml(content, [ + (div) => wrapMediaCards(div, postUrl), + addTargetBlankToLinks + ]); }; export const openLinksInNewTab = (content: string) => { - // Create a temporary div to parse the HTML - const div = document.createElement('div'); - div.innerHTML = content; - - // Find all anchor tags - const links = div.getElementsByTagName('a'); - - // Add target="_blank" and rel attributes to each link - for (let i = 0; i < links.length; i++) { - links[i].setAttribute('target', '_blank'); - links[i].setAttribute('rel', 'noopener noreferrer'); - } - - return div.innerHTML; + return processHtml(content, [addTargetBlankToLinks]); };Also applies to: 44-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(5 hunks)apps/admin-x-activitypub/src/utils/content-formatters.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (1)
apps/admin-x-activitypub/src/utils/content-formatters.ts (1) (1)
formatArticle(5-42)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
🔇 Additional comments (5)
apps/admin-x-activitypub/src/utils/content-formatters.ts (1)
5-42: Good temporary solution for handling media cardsThe implementation provides a good user experience improvement by allowing users to open media cards in the original article. The approach of wrapping audio and video cards in clickable anchors is straightforward and effective.
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (4)
23-23: Good integration of the formatting utilityProperly importing the new
formatArticlefunction to replace the previous link handling approach.
49-50: Interface properly updated with new parameterThe component interface correctly includes the new
postUrlparameter and its corresponding destructuring in the component parameters.Also applies to: 61-62
184-184: Good replacement of HTML formatting approachUsing the new
formatArticlefunction with both content and post URL parameters to improve media card handling.
935-936: Sensible fallback for post URLUsing
object?.url || ''provides a reasonable fallback when the URL is missing. TheformatArticlefunction properly handles this case by checking ifpostUrlexists before attempting to wrap media cards.
1 participant
ref AP-916, AP-915