ActivityPub UI design refinements #22242
Conversation
|
Warning Rate limit exceeded@peterzimon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes focus on several components within the admin activity stream layout, specifically the Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
529d4fa to
46fb73d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (1)
268-268: Consider extracting magic numbers into CSS variables.The padding values (
p-6 px-8 pb-[18px]) could be better managed using CSS variables for improved maintainability.- <div className={`group/article relative -mx-4 -my-px cursor-pointer rounded-lg p-6 px-8 pb-[18px]`} data-layout='feed' data-object-id={object.id} onClick={onClick}> + <div + className={` + group/article relative -mx-4 -my-px cursor-pointer rounded-lg + p-6 px-8 pb-[--feed-item-bottom-padding] + `} + style={{ '--feed-item-bottom-padding': '18px' }} + data-layout='feed' + data-object-id={object.id} + onClick={onClick} + >apps/admin-x-activitypub/src/components/layout/Header.tsx (3)
9-9: Consider addressing the TODO comment.The comment indicates a pending migration to React Router. This technical debt should be tracked and addressed in a future update.
Would you like me to create an issue to track this React Router migration task?
13-31: Remove unnecessary break statements after return.The break statements after return in the switch case are unreachable and can be safely removed.
Apply this diff to improve the code:
case 'inbox': return 'Inbox'; - break; case 'feed': return 'Feed'; - break; case 'profile': return 'Profile'; - break; case 'notifications': return 'Notifications'; - break; case 'search': return 'Search'; - break; default: return 'Inbox'; - break;🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 17-17: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 20-20: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 23-23: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 26-26: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 30-30: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
44-46: Consider error handling for route updates.The
updateRoutecalls could benefit from error handling to gracefully manage navigation failures.Consider wrapping the
updateRoutecalls in try-catch blocks:-onClick={() => { - updateRoute(''); -}} +onClick={async () => { + try { + await updateRoute(''); + } catch (error) { + console.error('Failed to update route:', error); + // Handle error appropriately + } +}}Also applies to: 63-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(1 hunks)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(3 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx(1 hunks)apps/admin-x-activitypub/src/components/layout/Header.tsx(2 hunks)apps/admin-x-activitypub/src/components/layout/Sidebar/Sidebar.tsx(2 hunks)apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(3 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-activitypub/src/components/layout/Sidebar/Sidebar.tsx
🔇 Additional comments (9)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (2)
254-254: LGTM! Improved button styling consistency.The conditional border radius for the UserMenuTrigger button enhances visual consistency across different layouts.
271-271: LGTM! Improved text sizing consistency.The text size adjustments to
text-smfor usernames and timestamps enhance readability and maintain visual hierarchy.Also applies to: 283-283
apps/admin-x-activitypub/src/components/layout/Header.tsx (2)
46-46: LGTM! Icon stroke width adjustment for better visibility.The subtle increase in stroke width from 1 to 1.1 improves the visual weight of the back arrow icon, aligning with the PR's UI refinement objectives.
63-63: LGTM! Comprehensive button styling improvements.The button styling changes enhance visual consistency and user experience through:
- Normalized font weight
- Consistent text size
- Improved dark mode hover states
- Refined dimensions and spacing
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (3)
6-8: LGTM! Good addition of account data loading.The addition of the account loading functionality and routing hook enhances the user experience by showing the account name and handling navigation properly.
Also applies to: 15-16
75-79: LGTM! Enhanced layout with loading state.Good implementation of the loading state using Skeleton component and improved spacing with flex layout.
Also applies to: 84-84
61-61:❓ Verification inconclusive
Verify modal width and button changes.
The modal width has been increased and the cancel button label has been removed. Please ensure these UI changes:
- Align with the design system's guidelines
- Maintain consistency across other modals in the application
Also applies to: 67-68
🏁 Script executed:
#!/bin/bash # Search for other modal width definitions and cancel button configurations rg -A 2 'Modal.*width=' rg -A 2 'Modal.*cancelLabel'Length of output: 56
Manual verification required for modal UI updates
The recent changes in
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(lines 61 and 67-68) indicate that the modal width has been increased and the cancel button label has been removed. Since our automated search for similar references (i.e.Modal.*width=andModal.*cancelLabel) did not yield any output, please manually verify that:
- The increased modal width conforms to the design system’s guidelines.
- The removal of the cancel button label maintains consistency with other modals in the application.
Given the absence of automated results, it's advisable to perform a detailed manual review of these UI aspects.
apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx (2)
69-69: LGTM! Improved layout width.The increased width provides better content visibility while maintaining readability.
70-77: Verify button styling accessibility.The new post button implementation has several accessibility considerations to verify:
- Color contrast for the placeholder text
- Focus state visibility
- Touch target size
#!/usr/bin/env python3 import requests from wcag_contrast_ratio import contrast # Check color contrast text_color = "#6B7280" # gray-500 bg_color = "#FFFFFF" # white dark_text_color = "#6B7280" # gray-500 dark_bg_color = "#000000" # black print(f"Light theme contrast ratio: {contrast(text_color, bg_color)}") print(f"Dark theme contrast ratio: {contrast(dark_text_color, dark_bg_color)}") # Verify touch target size button_height = 72 # px min_touch_target = 44 # px (WCAG 2.5.5 Target Size) print(f"Touch target size compliance: {button_height >= min_touch_target}")[skip_cloning]
Additionally, consider adding an aria-haspopup attribute since this button opens a modal:
-<Button aria-label='New post' className='text inset-0 h-[72px] w-full justify-start rounded-lg bg-white pl-[68px] text-left text-[1.5rem] font-normal tracking-normal text-gray-500 shadow-[0_5px_24px_0px_rgba(0,0,0,0.02),0px_2px_5px_0px_rgba(0,0,0,0.07),0px_0px_1px_0px_rgba(0,0,0,0.25)] transition-all hover:bg-white hover:shadow-[0_5px_24px_0px_rgba(0,0,0,0.05),0px_14px_12px_-9px_rgba(0,0,0,0.07),0px_0px_1px_0px_rgba(0,0,0,0.25)] dark:border dark:border-gray-950 dark:bg-black dark:shadow-none dark:hover:border-gray-925 dark:hover:bg-black dark:hover:shadow-none' onClick={() => NiceModal.show(NewPostModal)}> +<Button aria-label='New post' aria-haspopup='dialog' className='text inset-0 h-[72px] w-full justify-start rounded-lg bg-white pl-[68px] text-left text-[1.5rem] font-normal tracking-normal text-gray-500 shadow-[0_5px_24px_0px_rgba(0,0,0,0.02),0px_2px_5px_0px_rgba(0,0,0,0.07),0px_0px_1px_0px_rgba(0,0,0,0.25)] transition-all hover:bg-white hover:shadow-[0_5px_24px_0px_rgba(0,0,0,0.05),0px_14px_12px_-9px_rgba(0,0,0,0.07),0px_0px_1px_0px_rgba(0,0,0,0.25)] dark:border dark:border-gray-950 dark:bg-black dark:shadow-none dark:hover:border-gray-925 dark:hover:bg-black dark:hover:shadow-none' onClick={() => NiceModal.show(NewPostModal)}>
ref AP-748 - Some design details have to be updated for a more consistent and solid UX/visual.
691ea71 to
531d23a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (1)
61-61: Consider accessibility implications of empty cancel label.While the modal's visual refinements look good, having an empty
cancelLabelmight affect accessibility. Consider providing a meaningful label for screen readers while maintaining the desired visual appearance.- cancelLabel="" + cancelLabel="Cancel" + hideLabels={true}Also applies to: 67-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/admin-x-activitypub/src/components/activities/ActivityItem.tsx(1 hunks)apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(1 hunks)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(3 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx(1 hunks)apps/admin-x-activitypub/src/components/layout/Header.tsx(2 hunks)apps/admin-x-activitypub/src/components/layout/Sidebar/Sidebar.tsx(2 hunks)apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(3 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(1 hunks)apps/admin-x-activitypub/src/views/Notifications/Notifications.tsx(1 hunks)apps/admin-x-activitypub/src/views/Profile/Profile.tsx(2 hunks)apps/admin-x-activitypub/src/views/Search/Search.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/admin-x-activitypub/src/views/Search/Search.tsx
- apps/admin-x-activitypub/src/components/activities/ActivityItem.tsx
- apps/admin-x-activitypub/src/views/Profile/Profile.tsx
- apps/admin-x-activitypub/src/views/Notifications/Notifications.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx
- apps/admin-x-activitypub/src/components/layout/Header.tsx
- apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
- apps/admin-x-activitypub/src/components/layout/Sidebar/Sidebar.tsx
🔇 Additional comments (4)
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (4)
6-8: LGTM! New imports enhance the modal's functionality.The added imports for Skeleton component, ActivityPub queries hooks, and routing utilities align well with the new features being implemented.
15-16: LGTM! Proper implementation of routing and account data loading.The addition of routing and account data loading hooks enhances the component's functionality while following React best practices.
29-38: LGTM! Proper route update after successful post.The implementation correctly updates the route to 'feed' after a successful post, providing a smooth user experience.
75-79: LGTM! Clean implementation of account name display with loading state.The conditional rendering with Skeleton component provides a smooth loading experience while maintaining visual consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/shade/styles.css (1)
67-100: Updated Dark Mode Color Variables
The CSS variables within the dark mode block have been updated with new HSL values to support the refreshed design. Please verify that these new values provide the intended contrast and maintain accessibility standards across all affected components. Additionally, confirm consistency with the overall design system for dark mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(2 hunks)apps/admin-x-activitypub/src/components/layout/Header.tsx(2 hunks)apps/admin-x-activitypub/src/components/layout/Sidebar/Sidebar.tsx(2 hunks)apps/admin-x-activitypub/src/components/layout/Sidebar/SidebarButton.tsx(2 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(1 hunks)apps/shade/styles.css(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/admin-x-activitypub/src/components/layout/Header.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
- apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx
- apps/admin-x-activitypub/src/components/layout/Sidebar/Sidebar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Lint
🔇 Additional comments (5)
apps/admin-x-activitypub/src/components/layout/Sidebar/SidebarButton.tsx (2)
4-7: Well-structured component with proper TypeScript implementation!The component follows React best practices with proper type definitions, ref forwarding, and clear prop interface.
Also applies to: 9-10
15-16: Verify color contrast ratios for accessibility.While the styling changes enhance visual consistency, please ensure the color combinations meet WCAG accessibility standards, particularly:
- Text color (
text-gray-500) against background (bg-gray-925/70) in dark mode- White text against the semi-transparent background in active state
#!/usr/bin/env python3 from colorsys import rgb_to_hls import math def hex_to_rgb(hex_color): hex_color = hex_color.lstrip('#') return tuple(int(hex_color[i:i+2], 16) for i in (0, 2, 4)) def get_relative_luminance(r, g, b): r = r / 255 g = g / 255 b = b / 255 r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4 g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4 b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4 return 0.2126 * r + 0.7152 * g + 0.0722 * b def get_contrast_ratio(color1, color2, opacity=1.0): # Convert hex to RGB rgb1 = hex_to_rgb(color1) rgb2 = hex_to_rgb(color2) # Calculate luminance l1 = get_relative_luminance(*rgb1) l2 = get_relative_luminance(*rgb2) # Apply opacity to background if opacity < 1.0: l2 = l2 * opacity + l1 * (1 - opacity) # Calculate contrast ratio lighter = max(l1, l2) darker = min(l1, l2) return (lighter + 0.05) / (darker + 0.05) # Test color combinations # Assuming Tailwind's gray-500 is #6B7280 # Assuming gray-925 is close to #111111 text_color = "#6B7280" # gray-500 bg_color = "#111111" # gray-925 white = "#FFFFFF" # Calculate contrast ratios print(f"Dark mode text contrast ratio: {get_contrast_ratio(text_color, bg_color, 0.7):.2f}") print(f"Active state text contrast ratio: {get_contrast_ratio(white, bg_color, 0.7):.2f}") # WCAG 2.1 requirements: # - AA requires 4.5:1 for normal text # - AAA requires 7:1 for normal textapps/shade/styles.css (3)
148-150: Apply Text Color for Dark Mode Shade
The selector.dark .shadenow assigns a text color of#fafafb, which should improve readability against the updated dark backgrounds. Ensure that this color choice meets the required contrast ratios and aligns with the overall dark mode design.
152-154: Dark Mode Loading Orb Container Styling
The updated rule for.dark .shade .gh-loading-orb-containernow sets the background color to#000000. This appears to create a strong, clear backdrop for loading animations in dark mode. Please verify that this styling choice is consistent with the intended design and does not cause any unintended visual issues.
156-158: Ensure Proper Inversion for Loading Orb Icon
The rule for.dark .shade .gh-loading-orbappliesfilter: invert(100%);to the loading orb icon, which is intended to enhance its visibility on a dark background. Please confirm that this 100% inversion preserves the icon’s design integrity and does not adversely affect its clarity or accessibility.
ref AP-748