Conversation
This is a followup to [Julian's comment](bitfocus#3943 (comment)) in PR bitfocus#3943 Add download, companion website, and github links to the navbar, similar to the website repo. The companion website logo was the best I could think of to differentiate it from the main logo. An alternative would be the [bitfocus logo](https://bitfocus.io/assets/branding/logo-dark.svg) either with or without the text, though I don't think it is quite as recognizable out of context, yet.
|
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:
📝 WalkthroughWalkthroughAdds three right-aligned navbar items to Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/docusaurus.config.ts (1)
70-70: Consider using direct URLs instead of short links for long-term maintainabilitySmall thought — short URLs like
bfoc.us/djzdpq4g9gandbfoc.us/4orxauukegare handy but they obscure the actual destination, and if the shortener service ever goes down or the links expire, these would silently break. Would you consider using the direct GitHub/download URLs instead? Totally up to you though — if the team prefers short links for tracking purposes, that makes sense too! 🙂Also applies to: 89-89
|
@Julusian do you have an opinion on the rabbit's nitpick over "short urls" or conversely, should the link to the Companion website be replaced by a short URL? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/docusaurus.config.ts (1)
87-107: Facebook and GitHub items look good! Just a tiny formatting nit if you're up for it.The Facebook SVG block (line 93) has the closing
</svg>and<span>on the same line, while the Download (line 75-76) and GitHub (line 104-105) items have them on separate lines. Totally cosmetic, but aligning them would keep things tidy.Also noticed the FontAwesome version comments differ:
v7.1.0for Download/GitHub vsv7.2.0for Facebook. Might be intentional if you grabbed the icons at different times, but worth noting in case you'd like to keep them consistent for future updates.
update was a bit of a coderabbit time-waster (the icons did not change from v7.1 to v7.2). Though it does point out a potential problem of hardcoding the icons. I wasted a bit of time trying to use non-hardcoded, but gave up in the end.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/docusaurus.config.ts (1)
5-6: Unused imports — these can be removed 🧹It looks like you switched to hardcoded SVG strings (totally understandable given the troubleshooting you mentioned!), but the FontAwesome JS imports stuck around. None of
faSlack,faFacebook,faGithub, orFontAwesomeIconare referenced anywhere in the file now. Also,faSlackdoesn't correspond to any navbar item even conceptually.Removing these will keep the config clean and avoid pulling in unnecessary dependencies at build time.
Suggested fix
-import { faSlack, faFacebook, faGithub } from '@fortawesome/free-brands-svg-icons' -import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/docusaurus.config.ts (1)
70-70: Minor note on URL style consistency — already under discussion! 😊I see three items use
bfoc.usshort URLs while the Website link (line 83) uses the directbitfocus.ioURL. Looks like you've already raised this with the maintainer in the PR comments, so just flagging it here for visibility. Either approach works — short URLs are tidier but direct URLs are more transparent to users who hover over links.
|
Here's another option that looks less cluttered than a long array of icons... 
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/docusaurus.config.ts (1)
87-137: The Support dropdown is a clean way to organize these links — nicely done! 🙌A couple of small things worth considering:
aria-labelconsistency in dropdown items: Same note as the Download item — the Facebook, Slack, GitHub, and Donate sub-items havearia-labelon the SVGs but not on the link elements themselves. If you add it to the Download item, it'd be nice to match here too.Opaque short URLs: The
bfoc.uslinks are handy for tracking but make it harder for reviewers (and future maintainers) to know where each link goes at a glance. A short inline comment next to each href (e.g.,// -> facebook.com/groups/...) would be a friendly breadcrumb for anyone reading this config later.Neither of these is blocking — just ideas to polish things up. Thanks for putting this together! 🎉
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/docusaurus.config.ts (1)
88-143: The Support dropdown is well-organized — love the grouping! 🎉The dropdown structure with Facebook, Slack, GitHub, a divider, and Donate is intuitive and keeps the navbar tidy. All items have consistent
aria-label,className, andhtmlpatterns, which is great for both accessibility and maintainability.One totally optional thought for the future: if the number of inline SVGs keeps growing, you could consider extracting them into a small helper (e.g.,
const svgIcon = (path, label) => ...) to reduce the visual noise in the config. But honestly, it's perfectly fine as-is for this scope.
I haven't had a chance to look at the full changes yet, but one quick thing based on that image is we should change the wording from 'Donate' to something more appropriate. The Open Collective is not a registered charity and neither us who use the fun or the people who contribute are provided the same legal and tax benefits of charitable donations. This is something that has been brought up in the past but not something actively worked on fixing, but since this area is being worked on then it's a good opportunity to appropriately update the wording in this areas. Something like "Contribute to Companion", "Tip Companion", "Support Companion", something along those lines that wouldn't have the same association with a charity. |
|
If we can decide on terminology to use, then it will be pretty quick to change it all. |
|
Sponsor would work, as that would also match the terminology of module devs who use Github sponsorships. |
|
So I consulted Google, Claude, Open Collective, a dictionary and my own AI (Ari's Intelligence)... Sponsor and Support are among their top favorites. Maybe "Become a Sponsor" or "Sponsor Companion" but as one of the sources points out, "Sponsor" often refers to high-value contributors, so may not be the best choice (depending on your intentions; Claude: "sponsor" fits organizational/higher-tier giving.). "Support Companion" sounds good and it's a bit clever, since it's included in the "Support" section. My top pick, though is to replace it with two "Contribute" entries:
This solves two problems - encouraging "donations" and encouraging people to contribute code. Other fun ones could be "Become a Backer" or "Show your love" 😀. FWIW, purely academically, "Donate" is still technically correct although, as Jeff pointed out people do tend to associate it more with charity. (I've certainly encountered enough solicitations that say "your donation is not tax-deductible", to separate the two concepts in my mind.) Regardless, I like the two contributes much better: it sound much more collaborative. Bottom line, any of these phrases will do. If you two agree on a phrase, I can fix it here for the packaged docs and for Header.tsx, and then fix it in the sidebar along with the context-menu pr, which includes sidebar mods. And probably it would be worth porting it back to the website as well... |
(sync with website)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/src/css/custom.css (2)
147-149: Comment is now slightly out-of-date after the selector was expandedThe comment on line 147 still says "when the window is narrow, the right-header icons are in the pull-down sidebar", but the newly-added
.theme-layout-navbar-right .dropdown__menuselector targets the dropdown that appears in the wide view (the right navbar area), not just the narrow pull-down sidebar. A small wording tweak would keep the code easy to follow for future readers:📝 Suggested comment update
-/* when the window is narrow, the right-header icons are in the pull-down sidebar */ +/* fontawesome-container layout for: + - narrow windows: icons appear in the pull-down sidebar (.navbar-sidebar__items) + - wide windows: icons appear inside right-navbar dropdowns (.theme-layout-navbar-right .dropdown__menu) */
221-231:&::beforeinside.menu__link--sublist-caret::aftergenerates an invalid compound pseudo-element ruleWhen postcss-preset-env expands the nested
&::beforeblock inside the combined selector, it produces two rules:
.menu__caret::before { scale: 0.6; }— valid ✓.menu__link--sublist-caret::after::before { scale: 0.6; }— invalid (CSS doesn't allow chaining::after::before)Browsers silently discard rule 2, so nothing visually breaks — the scale for the no-linked-page caret is already handled correctly by the standalone rule at lines 234-236. But the nested block is dead code for that second selector and can cause confusion.
The cleanest fix is to restrict the
&::beforenesting to only the.menu__caretcase:♻️ Proposed refactor
-.menu__caret, -.menu__link--sublist-caret::after { +.menu__caret { order: -1; padding-left: 0.45em; padding-right: 0; border-radius: 0.5rem 0 0 0.5rem; &::before { scale: 0.6; } } + +.menu__link--sublist-caret::after { + order: -1; + padding-left: 0.45em; + padding-right: 0; + border-radius: 0.5rem 0 0 0.5rem; +}The standalone scale rule below (lines 234-236) already handles the
::aftercase, so no other changes are needed.
(sync with website repo)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/src/css/custom.css (1)
147-151: Nice expansion of the selectors — one small note on.navbar__inner👋The added comment explaining the two contexts is really helpful, thank you! One thing to keep in mind:
.navbar__inneris an Infima implementation-detail class rather than a documented Docusaurus theme class. In a previous discussion, the preference was to use stable, documented selectors like.theme-layout-navbar-rightwhere possible to avoid breakage on Infima updates.That said, I'm not sure there's a documented theme-level class for "dropdown menus within the navbar," so this may be the best available option. Just something to be aware of if a future Infima upgrade shuffles internal class names.
Based on learnings: "In Docusaurus docs, prefer using documented CSS classes such as .theme-layout-navbar-right to target the right navbar area rather than implementation-specific class names like .navbar__items--right."
3 participants
This is a followup to Julian's comment in PR #3943
Add download, companion website, and github links to the navbar, similar to the website repo.
The companion website logo was the best I could think of to differentiate it from the main logo. An alternative would be the bitfocus logo, either with or without the text, though I don't think it is quite as recognizable out of context, yet.
(FWIW, the transparent "padding" in the PNG file is needed to match FontAwesome's format.)
Summary by CodeRabbit
New Features
Style