-
Notifications
You must be signed in to change notification settings - Fork 37
Enhance Daily Notes Popup with date banners for headings. #507
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
Conversation
- Fix bug introduced by Roam DOM changes - Introduced utility functions for parsing dates and creating banners, and implemented an observer for dynamic heading updates. - Refactored initialization logic to improve readability and maintainability.
|
Caution Review failedThe pull request is closed. WalkthroughAdds Daily Note Subtitles/Banner: parses dates from H1 headings, creates/inserts day banners, detects duplicates, processes existing headings, and observes new headings via createHTMLObserver. Also bumps package version from 1.7.0 to 1.7.1 and refactors initialization/cleanup to use the new observer lifecycle when enabled. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as App Init
participant Settings as User Settings
participant DOM as Document
participant Observer as createHTMLObserver
participant Banner as Banner Utils
App->>Settings: Read dailySubtitles setting
alt dailySubtitles != "off"
App->>DOM: processExistingHeadings()
loop For each heading
DOM->>Banner: parseDateFromHeading(h)
alt date parsed and no existing banner
Banner-->>DOM: createDayBanner(date)
DOM->>DOM: insertBanner(after title)
else No-op
end
end
App->>Observer: setupHeadingObserver()
Observer-->>App: observing
Note right of Observer: On new/modified H1 nodes
Observer->>Banner: parseDateFromHeading(new H1)
alt valid and not duplicated
Banner-->>DOM: createDayBanner(date)
DOM->>DOM: insertBanner(after title)
else No-op
end
else
App->>App: Skip banner setup
end
rect rgba(220,240,255,0.5)
Note over App,Observer: Toggle off triggers cleanupHeadingObserver()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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. Comment |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/features/dailyNotesPopup.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/dailyNotesPopup.tsx (1)
src/settings.ts (1)
get(6-21)
RE: https://roamresearch.slack.com/archives/CN5MK4D2M/p1759244379313669
This was caused by Roam changing the DOM and wrapping the
h1.rm-title-displayin adiv.rm-title-display-containerflex container for Pages and Sidebar windows. DNP is currently unaffected.This is still in draft because they are considering wrapping the DNP
h1in the same<div class="rm-title-display-container">for a consistent DOM structure for dev's to hook into.UPDATE
<div class="rm-title-display-container">is now on all casesSummary by CodeRabbit
New Features
Chores