Increase navbar logo size and brand presence#22
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced navbar favicon/logo and fixed logo sizing; expanded global and responsive CSS; added a two-column decision grid and updated examples markup; reworked homepage hero to use useBaseUrl and changed eyebrow elements to h3; changed docs prep to accept and transform upstream README content. Changes
Sequence Diagram(s)sequenceDiagram
participant Upstream as Upstream README
participant Script as prepare-docs.mjs
participant FS as Filesystem (docs target)
Upstream->>Script: provide sourceMarkdown
Script->>Script: docsHomeMarkdown(sourceMarkdown, {hasArchive})
Script->>FS: write transformed README with frontmatter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Redundant double spacing from gap and margin-right
- Removed the redundant
.navbar__logoright margins on desktop and mobile so spacing is controlled solely by.navbar__brandgap.
- Removed the redundant
Or push these changes by commenting:
@cursor push 33399a8303
Preview (33399a8303)
diff --git a/prototypes/docusaurus/src/css/custom.css b/prototypes/docusaurus/src/css/custom.css
--- a/prototypes/docusaurus/src/css/custom.css
+++ b/prototypes/docusaurus/src/css/custom.css
@@ -94,7 +94,6 @@
.navbar__logo {
width: 2.15rem;
height: 2.15rem;
- margin-right: 0.3rem;
}
.navbar__logo img {
@@ -249,7 +248,6 @@
.navbar__logo {
width: 1.95rem;
height: 1.95rem;
- margin-right: 0.22rem;
}
.navbar__title {
|
Cloudflare preview deployed.
|
justin808
force-pushed
the
jg-codex/navbar-logo-scale
branch
from
24d7013 to
8189a14
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
prototypes/docusaurus/src/css/custom.css (1)
88-104: Minor redundancy: bothgapandmargin-rightcreate spacing between logo and title.
.navbar__brandhasgap: 0.28remwhile.navbar__logohasmargin-right: 0.35rem. Both properties add spacing between the logo and title, totaling ~0.63rem. If intentional for fine-grained control, consider adding a brief comment. Otherwise, using onlygapon the flex container would be cleaner.♻️ Optional: Use gap only for spacing
.navbar__brand { display: inline-flex; align-items: center; - gap: 0.28rem; + gap: 0.5rem; } .navbar__logo { width: 2.35rem; height: 2.35rem; - margin-right: 0.35rem; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prototypes/docusaurus/src/css/custom.css` around lines 88 - 104, The spacing is duplicated: .navbar__brand sets gap: 0.28rem while .navbar__logo applies margin-right: 0.35rem, resulting in doubled spacing; remove the margin-right from .navbar__logo and rely solely on the flex container gap for spacing (or, if the combined spacing is intentional, keep both but add a brief comment near .navbar__brand/.navbar__logo explaining why both gap and margin-right are used for fine‑tuning).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prototypes/docusaurus/docusaurus.config.ts`:
- Around line 69-72: The config sets logo attributes src: 'img/logo-mark.png'
with width: 64 and height: 64 but the PR description states 40; update the
config attributes (width and height) to 40 to match the PR and CSS intent (or
alternatively update the PR description if 64 was intentional) so the
navbar.logo fallback HTML dimensions (width and height) align with the
documented target.
---
Nitpick comments:
In `@prototypes/docusaurus/src/css/custom.css`:
- Around line 88-104: The spacing is duplicated: .navbar__brand sets gap:
0.28rem while .navbar__logo applies margin-right: 0.35rem, resulting in doubled
spacing; remove the margin-right from .navbar__logo and rely solely on the flex
container gap for spacing (or, if the combined spacing is intentional, keep both
but add a brief comment near .navbar__brand/.navbar__logo explaining why both
gap and margin-right are used for fine‑tuning).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c33beb6f-534d-4f3b-b158-aec4a63098a8
⛔ Files ignored due to path filters (1)
prototypes/docusaurus/static/img/logo-mark.pngis excluded by!**/*.png
📒 Files selected for processing (2)
prototypes/docusaurus/docusaurus.config.tsprototypes/docusaurus/src/css/custom.css
justin808
force-pushed
the
jg-codex/navbar-logo-scale
branch
from
8189a14 to
8ec9572
Compare
|
Resolved the branch conflicts and folded the current-main follow-ups from issue #24 into this branch. One merge-order note: this branch now reads the docs-home content from |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Heading element inherits serif font on eyebrow label
- I changed the flow card eyebrow element back to a
<p>so it inherits the body sans-serif font and no longer picks up.flowCard h3heading overrides.
- I changed the flow card eyebrow element back to a
Or push these changes by commenting:
@cursor push f1ef1af661
Preview (f1ef1af661)
diff --git a/prototypes/docusaurus/src/pages/index.tsx b/prototypes/docusaurus/src/pages/index.tsx
--- a/prototypes/docusaurus/src/pages/index.tsx
+++ b/prototypes/docusaurus/src/pages/index.tsx
@@ -170,7 +170,7 @@
<div className={styles.flowGrid}>
{recommendedFlows.map((flow) => (
<article className={styles.flowCard} key={flow.title}>
- <h3 className={styles.cardEyebrow}>{flow.title}</h3>
+ <p className={styles.cardEyebrow}>{flow.title}</p>
<p className={styles.flowSummary}>{flow.summary}</p>
<code className={styles.inlineCode}>{flow.command}</code>
<Link className={styles.cardLink} to={flow.href}>
justin808
force-pushed
the
jg-codex/navbar-logo-scale
branch
from
4eaaa46 to
fc4b0fd
Compare
|
Rebased this branch onto current |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/prepare-docs.mjs (2)
521-526: Add explicit existence validation for source docs home README.If upstream moves/renames the README, this currently fails with a raw read error. A targeted
ensureExistscheck gives a clearer failure mode.Proposed improvement
- const docsHomeSource = await fs.readFile(path.join(sourceDocs, "README.md"), "utf8"); + const docsHomePath = path.join(sourceDocs, "README.md"); + await ensureExists( + docsHomePath, + `Expected docs home README at ${docsHomePath}. Check upstream docs layout before preparing.` + ); + const docsHomeSource = await fs.readFile(docsHomePath, "utf8");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/prepare-docs.mjs` around lines 521 - 526, Add an explicit existence check before reading the docs home README: instead of calling fs.readFile directly for path.join(sourceDocs, "README.md") (which assigns docsHomeSource), first call a clear existence validator (e.g., a small ensureExists helper or fs.access/fs.stat) to verify the file exists and throw a descriptive error if it does not; then proceed to read the file and write using docsHomeMarkdown as before so failures report “missing README in sourceDocs” rather than a raw read error.
470-478: Make archive-link injection resilient to upstream heading formatting changes.The
Need more help?insertion currently relies on one exact string, so a small upstream README formatting change can silently drop the archive link.Proposed hardening
function docsHomeMarkdown(sourceMarkdown, { hasArchive }) { const archiveBlock = hasArchive ? "- [Historical Reference](./archive/README.md)\n" : ""; - const updated = sourceMarkdown + let updated = sourceMarkdown .trim() .replaceAll("(./oss/", "(./") .replace("](https://reactonrails.com/examples)", "](/examples)") .replace(/\n- \[Documentation website\]\(https:\/\/reactonrails\.com\/docs\/\)\s*/g, "\n") - .replace("## Need more help?\n\n", `## Need more help?\n\n${archiveBlock}`); + .replace(/\r\n/g, "\n"); + + if (hasArchive) { + if (/^##\s+Need more help\?\s*$/m.test(updated)) { + updated = updated.replace( + /^##\s+Need more help\?\s*$/m, + "## Need more help?\n\n- [Historical Reference](./archive/README.md)" + ); + } else { + updated += "\n\n## Need more help?\n\n- [Historical Reference](./archive/README.md)\n"; + } + } return `---\ncustom_edit_url: null\n---\n\n${updated}\n`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/prepare-docs.mjs` around lines 470 - 478, The insertion in docsHomeMarkdown is brittle because it matches the exact string "## Need more help?\n\n"; change the replacement to use a regex that finds the "Need more help?" heading with flexible whitespace/newlines (e.g. match /^##\s*Need more help\?\s*(\r?\n)+/m or a lookahead) and inject archiveBlock right after the heading capture so variations in upstream heading formatting won't skip the archive link; update the .replace call on updated (in docsHomeMarkdown) to perform this regex-based insertion using the captured heading/newline group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/prepare-docs.mjs`:
- Around line 521-526: Add an explicit existence check before reading the docs
home README: instead of calling fs.readFile directly for path.join(sourceDocs,
"README.md") (which assigns docsHomeSource), first call a clear existence
validator (e.g., a small ensureExists helper or fs.access/fs.stat) to verify the
file exists and throw a descriptive error if it does not; then proceed to read
the file and write using docsHomeMarkdown as before so failures report “missing
README in sourceDocs” rather than a raw read error.
- Around line 470-478: The insertion in docsHomeMarkdown is brittle because it
matches the exact string "## Need more help?\n\n"; change the replacement to use
a regex that finds the "Need more help?" heading with flexible
whitespace/newlines (e.g. match /^##\s*Need more help\?\s*(\r?\n)+/m or a
lookahead) and inject archiveBlock right after the heading capture so variations
in upstream heading formatting won't skip the archive link; update the .replace
call on updated (in docsHomeMarkdown) to perform this regex-based insertion
using the captured heading/newline group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06e600d8-b158-4a9c-a609-4c62da21ed89
⛔ Files ignored due to path filters (1)
prototypes/docusaurus/static/img/logo-mark.pngis excluded by!**/*.png
📒 Files selected for processing (7)
prototypes/docusaurus/docusaurus.config.tsprototypes/docusaurus/src/css/custom.cssprototypes/docusaurus/src/pages/examples.module.cssprototypes/docusaurus/src/pages/examples.tsxprototypes/docusaurus/src/pages/index.module.cssprototypes/docusaurus/src/pages/index.tsxscripts/prepare-docs.mjs
✅ Files skipped from review due to trivial changes (3)
- prototypes/docusaurus/src/pages/examples.tsx
- prototypes/docusaurus/docusaurus.config.ts
- prototypes/docusaurus/src/pages/examples.module.css
🚧 Files skipped from review as they are similar to previous changes (1)
- prototypes/docusaurus/src/css/custom.css
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Double frontmatter if source README has existing frontmatter
docsHomeMarkdownnow detects existing frontmatter and injectscustom_edit_url: nullinto it instead of prepending a second frontmatter block.
Or push these changes by commenting:
@cursor push abd80b0354
Preview (abd80b0354)
diff --git a/scripts/prepare-docs.mjs b/scripts/prepare-docs.mjs
--- a/scripts/prepare-docs.mjs
+++ b/scripts/prepare-docs.mjs
@@ -477,7 +477,11 @@
.replace(/\n- \[Documentation website\]\(https:\/\/reactonrails\.com\/docs\/\)\s*/g, "\n")
.replace("## Need more help?\n\n", `## Need more help?\n\n${archiveBlock}`);
- return `---\ncustom_edit_url: null\n---\n\n${updated}\n`;
+ const withCustomEditUrl = updated.startsWith("---")
+ ? updated.replace(/^---\n/, "---\ncustom_edit_url: null\n")
+ : `---\ncustom_edit_url: null\n---\n\n${updated}`;
+
+ return `${withCustomEditUrl}\n`;
}
async function prepareDocusaurus() {| - If your organization is budget-constrained, contact us about free licenses. | ||
|
|
||
| ${archiveSection}`; | ||
| return `---\ncustom_edit_url: null\n---\n\n${updated}\n`; |
There was a problem hiding this comment.
Double frontmatter if source README has existing frontmatter
Low Severity
docsHomeMarkdown unconditionally prepends ---\ncustom_edit_url: null\n--- frontmatter to the source content. If the upstream README.md already contains frontmatter (starts with ---), the output will have double frontmatter, causing a Docusaurus parse error. The sibling function injectProFriendlyNotice in the same file already demonstrates the correct pattern — checking startsWith("---") before deciding whether to insert into existing frontmatter or add new frontmatter.
Additional Locations (1)
justin808
force-pushed
the
jg-codex/navbar-logo-scale
branch
from
fc4b0fd to
3ddbaa7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: CSS specificity override on eyebrow heading line-height
- I confirmed the specificity issue and added
line-height: 1.3to.flowCard .cardEyebrowso it reliably overrides.flowCard h3.
- I confirmed the specificity issue and added
Or push these changes by commenting:
@cursor push 61f69ae7a1
Preview (61f69ae7a1)
diff --git a/prototypes/docusaurus/src/pages/index.module.css b/prototypes/docusaurus/src/pages/index.module.css
--- a/prototypes/docusaurus/src/pages/index.module.css
+++ b/prototypes/docusaurus/src/pages/index.module.css
@@ -215,6 +215,7 @@
.flowCard .cardEyebrow {
margin-bottom: 0.7rem;
+ line-height: 1.3;
}
.inlineCode {|
|
||
| .flowCard .cardEyebrow { | ||
| margin-bottom: 0.7rem; | ||
| } |
There was a problem hiding this comment.
CSS specificity override on eyebrow heading line-height
Low Severity
The new .cardEyebrow rule sets line-height: 1.3, but changing the flow card eyebrow from <p> to <h3> causes it to also match the pre-existing .flowCard h3 selector, which has higher specificity (0,1,1 vs 0,1,0) and sets line-height: 1.12. The .flowCard .cardEyebrow override only fixes margin-bottom but misses line-height, so the eyebrow heading inherits the generic card heading line-height despite the stated intent to avoid that.
Additional Locations (2)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
prototypes/docusaurus/src/css/custom.css (1)
209-211: Consider using a CSS variable for the hardcoded hover color.Line 210 uses
#21262ddirectly while the rest of the dark mode styles use CSS variables. For consistency and easier theme maintenance, consider defining this as a variable.♻️ Optional: Extract to a CSS variable
[data-theme='dark'] { /* ... existing variables ... */ + --site-hover-surface: `#21262d`; }[data-theme='dark'] .button--secondary:hover { - background: `#21262d`; + background: var(--site-hover-surface); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prototypes/docusaurus/src/css/custom.css` around lines 209 - 211, Replace the hardcoded color in the selector [data-theme='dark'] .button--secondary:hover with a CSS variable: add a new variable (e.g., --color-button-secondary-hover-dark) to your dark theme variables block and use that variable in the rule (with an optional fallback like var(--color-button-secondary-hover-dark, `#21262d`)); update any other dark-mode button hover rules to reference the same variable for consistency and easier maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/prepare-docs.mjs`:
- Around line 553-560: The generated docs currently rewrite the upstream
Changelog link and then later deletes that target, leaving a broken internal
link; update the transformation that builds updated (based on sourceMarkdown and
archiveBlock) to either remove the changelog list item or retarget it to a
surviving page: add a .replace call in the chain that matches the markdown list
item linking to upgrading/changelog.md (e.g., match "-
[Changelog](./oss/upgrading/changelog.md)" or any variant under
upgrading/changelog.md) and replace it with either an empty string (to drop the
item) or a safe target like "](./upgrading)" or an absolute/raw url; modify the
code around the updated variable where sourceMarkdown is trimmed/replaced to
include this extra .replace so the final returned content no longer contains the
dead changelog link.
---
Nitpick comments:
In `@prototypes/docusaurus/src/css/custom.css`:
- Around line 209-211: Replace the hardcoded color in the selector
[data-theme='dark'] .button--secondary:hover with a CSS variable: add a new
variable (e.g., --color-button-secondary-hover-dark) to your dark theme
variables block and use that variable in the rule (with an optional fallback
like var(--color-button-secondary-hover-dark, `#21262d`)); update any other
dark-mode button hover rules to reference the same variable for consistency and
easier maintenance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4236e348-b3d8-46b3-a3c5-6e593f2e2ede
⛔ Files ignored due to path filters (1)
prototypes/docusaurus/static/img/logo-mark.pngis excluded by!**/*.png
📒 Files selected for processing (7)
prototypes/docusaurus/docusaurus.config.tsprototypes/docusaurus/src/css/custom.cssprototypes/docusaurus/src/pages/examples.module.cssprototypes/docusaurus/src/pages/examples.tsxprototypes/docusaurus/src/pages/index.module.cssprototypes/docusaurus/src/pages/index.tsxscripts/prepare-docs.mjs
✅ Files skipped from review due to trivial changes (3)
- prototypes/docusaurus/docusaurus.config.ts
- prototypes/docusaurus/src/pages/examples.tsx
- prototypes/docusaurus/src/pages/examples.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- prototypes/docusaurus/src/pages/index.tsx
- prototypes/docusaurus/src/pages/index.module.css
| const updated = sourceMarkdown | ||
| .trim() | ||
| .replaceAll("(./oss/", "(./") | ||
| .replace("](https://reactonrails.com/examples)", "](/examples)") | ||
| .replace(/\n- \[Documentation website\]\(https:\/\/reactonrails\.com\/docs\/\)\s*/g, "\n") | ||
| .replace("## Need more help?\n\n", `## Need more help?\n\n${archiveBlock}`); | ||
|
|
||
| ### Evaluating Rails + React options | ||
|
|
||
| - [Examples and migration references](/examples) | ||
| - [Migrate from react-rails](./migrating/migrating-from-react-rails.md) | ||
|
|
||
| ## Dive deeper when you need it | ||
|
|
||
| - [Introduction](./introduction.md) | ||
| - [Core Concepts](./core-concepts/how-react-on-rails-works.md) | ||
| - [API Reference](./api-reference/view-helpers-api.md) | ||
| - [Deployment and troubleshooting](./deployment/README.md) | ||
|
|
||
| ## Friendly evaluation policy | ||
|
|
||
| - You can try React on Rails Pro without a license while evaluating. | ||
| - If your organization is budget-constrained, contact us about free licenses. | ||
|
|
||
| ${archiveSection}`; | ||
| return `---\ncustom_edit_url: null\n---\n\n${updated}\n`; |
There was a problem hiding this comment.
Don't publish a changelog link that the same prepare step deletes.
Line 555 rewrites the upstream [Changelog](./oss/upgrading/changelog.md) entry to ./upgrading/changelog.md, and Line 597 then removes docs/upgrading/changelog.md. That leaves the generated docs home with a dead internal link. Filter that item out here or retarget it to a page that survives the prepare step. (raw.githubusercontent.com)
🩹 Suggested fix
const updated = sourceMarkdown
.trim()
.replaceAll("(./oss/", "(./")
.replace("](https://reactonrails.com/examples)", "](/examples)")
+ .replace(/\n\s*[-*] \[Changelog\]\(\.\/upgrading\/changelog\.md\)\s*/g, "\n")
.replace(/\n- \[Documentation website\]\(https:\/\/reactonrails\.com\/docs\/\)\s*/g, "\n")
.replace("## Need more help?\n\n", `## Need more help?\n\n${archiveBlock}`);Also applies to: 596-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/prepare-docs.mjs` around lines 553 - 560, The generated docs
currently rewrite the upstream Changelog link and then later deletes that
target, leaving a broken internal link; update the transformation that builds
updated (based on sourceMarkdown and archiveBlock) to either remove the
changelog list item or retarget it to a surviving page: add a .replace call in
the chain that matches the markdown list item linking to upgrading/changelog.md
(e.g., match "- [Changelog](./oss/upgrading/changelog.md)" or any variant under
upgrading/changelog.md) and replace it with either an empty string (to drop the
item) or a safe target like "](./upgrading)" or an absolute/raw url; modify the
code around the updated variable where sourceMarkdown is trimmed/replaced to
include this extra .replace so the final returned content no longer contains the
dead changelog link.
1 participant
Summary
Makes the top-left React on Rails logo more prominent in the navbar.
Changes
40x40) for stable rendering.Files
prototypes/docusaurus/docusaurus.config.tsprototypes/docusaurus/src/css/custom.cssVerification
npm run buildpasses locally.Note
Low Risk
Mostly visual/layout updates to Docusaurus pages plus a small docs-prep script change to source the docs landing page from upstream; low risk but could affect site appearance and generated
docs/README.mdcontent/links.Overview
Updates Docusaurus branding by switching the favicon/navbar logo to
img/logo-mark.png, setting explicit navbar logo dimensions, and adding CSS to increase logo size and tighten brand alignment (with responsive tweaks).Refreshes page layouts/styles: adds a branded hero identity block on the home page (logo + product name), adjusts grids on
Examplesand the home page for better responsiveness, tweaks dark-mode secondary button hover, and minor typography/polish (e.g., inline code radius, flow card eyebrow now rendered as anh3).Changes
scripts/prepare-docs.mjsso the generateddocs/README.mdis derived from the upstreamREADME.mdand then normalized (link rewrites + optional archive link), instead of using a hardcoded template.Written by Cursor Bugbot for commit 3ddbaa7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Style
Layout
Accessibility
Documentation