-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[PE-147] chore: merge two separate page info popovers into one #6289
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to page-related components, focusing on simplifying and restructuring user information display. The changes primarily affect the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
web/core/components/pages/dropdowns/edit-information-popover.tsx (1)
15-15
: Consider a fallback message for missing timestamps.
Ifpage.updated_at
is empty or undefined, the expressioncalculateTimeAgoShort(page.updated_at ?? "")
may result in an unclear time display (e.g., "Edited ago"). Consider a clearer fallback, such as "Just now" or similar.web/core/components/pages/editor/header/info-popover.tsx (2)
37-41
: User details resolution.
getUserDetails
usage is neat and straightforward. Consider a fallback scenario (e.g., displaying "User not found") if a user lookup fails.
94-131
: Enhanced user identification sections.
Providing detailed "Edited by" and "Created by" sections with clickable user avatars is a great addition for transparency and collaboration. Minor improvement: handle potential missing or invalid user avatar URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/core/components/pages/dropdowns/edit-information-popover.tsx
(2 hunks)web/core/components/pages/editor/header/extra-options.tsx
(1 hunks)web/core/components/pages/editor/header/info-popover.tsx
(2 hunks)
🔇 Additional comments (8)
web/core/components/pages/dropdowns/edit-information-popover.tsx (1)
3-4
: Well-structured import usage.
No issues found with these imports, and the calculateTimeAgoShort
helper is a sensible choice for handling relative timestamps.
web/core/components/pages/editor/header/extra-options.tsx (1)
87-87
: Prop injection looks good.
Passing page={page}
into PageInfoPopover
is a clean way to make relevant page data accessible within the popover. Ensure that page
is never null or undefined to prevent runtime errors.
web/core/components/pages/editor/header/info-popover.tsx (6)
2-3
: Consistent routing and linking approach.
The usage of Link
from "next/link"
and useParams
from "next/navigation"
aligns with Next.js best practices for client-side navigation.
8-11
: UI utility imports used appropriately.
Imports for Avatar
, getFileURL
, and other helpers look properly structured, keeping the external UI logic separate from component internals.
14-15
: Store integration is efficient.
Using the useMember
hook to retrieve user details keeps the logic clean. Just ensure that the store is ready when the component mounts.
21-25
: Additional prop for page data.
The page: TPageInstance
prop effectively provides the necessary data for the popover, centralizing the logic around the page
object.
31-32
: Workspace slug retrieval is straightforward.
Ensure workspaceSlug
is defined and non-empty in all usage scenarios. Otherwise, the profile links could break.
81-93
: Informative grid layout.
The structured grid for the document info cards (words, characters, paragraphs, read-time) is clear and user-friendly.
Description
This PR merges the two separate info popovers, one with the details of users who created and last edited the page and the other with information like character, word count, etc., into one.
Type of Change
Media
Summary by CodeRabbit
New Features
Refactor
UI Changes