Skip to content

feat(pages): add macports to pages homepage - #873

Merged
lizhengfeng101 merged 10 commits into
alibaba:mainfrom
wu21-web:mp
Aug 14, 2026
Merged

feat(pages): add macports to pages homepage#873
lizhengfeng101 merged 10 commits into
alibaba:mainfrom
wu21-web:mp

Conversation

@wu21-web

Copy link
Copy Markdown
Contributor

Description

Add a macports icon and put macports installation method right next to homebrew as a tab.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)
    Tested through npm install && npm run dev
    Here is what it look likes:
Screenshot 2026-08-12 at 8 00 44 PM

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

none yet

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 5 selected item(s).

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Thanks for putting this together — the icon and the tab layout look clean, and I appreciate you testing it locally.

That said, I'd rather not surface MacPorts on the homepage for now, for two reasons:

  • Maintenance. Our Homebrew formula lives in homebrew-core, so new releases get picked up more or less automatically. A MacPorts portfile would be one more thing to bump by hand every release, and that's exactly the kind of thing that quietly goes stale and ends up pointing people at an old version.
  • Reach. The MacPorts audience is a good deal smaller, so it's hard to justify the homepage real estate for it.

The install story we want to lead with is npm first, Homebrew second, and I'd like to keep the homepage down to just those two so the choice stays obvious for newcomers.

Sorry to turn this one down after the work you put in — it's not a knock on the patch itself. If MacPorts users find their way to a port, that's great, we just don't want to be the ones promising to keep it current.

@wu21-web

wu21-web commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 I maintain the macports port for this, and the newest now avaliable is v1.9.2, I submitted a portfile for v1.9.3
I watched the repository for tag publishes, and I did my best to keep the Portfile up to date.
This requires no maintainence for people at alibaba/open-code-review, because you have me as the BrewTestBot at macports.

@wu21-web

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101

lizhengfeng101 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Let me walk back my earlier comment — knowing you're the one maintaining the port changes my read on it, and I'd rather find MacPorts a home on the homepage than turn it away. Thanks for the patience while I thought it through.

Here's the constraint I was dancing around. We're about to add an install-script channel — curl -fsSL … | bash for macOS/Linux and the PowerShell equivalent for Windows — and once that lands, a flat tab row runs out of horizontal room. It's already tight at four labels in English, and ja/ru translations push it over. So rather than argue about which channels earn a tab, I'd like to change the shape of the control:

┌──────────────────────────────────────────────────────┐
│  ⬢ npm    🍺 Homebrew    ▢ Script    More ⌄          │
├──────────────────────────────────────────────────────┤          ┌───────────────────────┐
│  npm i -g @alibaba-group/open-code-review        📋  │          │  Ⓜ  MacPorts          │
└──────────────────────────────────────────────────────┘          │  ⊞  Windows           │
                                                                  ├───────────────────────┤
                                                                  │  All install options → │
                                                                  └───────────────────────┘

Three primary tabs stay flat, everything else moves behind More ⌄ — one extra click for the long tail, and the row stops growing every time we add a channel. MacPorts goes in the dropdown, which I think is the honest placement: discoverable and copy-pasteable, without claiming a slot next to npm.

Would you be up for implementing the dropdown as part of this PR? If so, a few notes so you're not guessing at my preferences:

  • Add a primary: boolean to INSTALL_CHANNELS and derive the two groups from it, so promoting or demoting a channel later is a one-word edit.
  • activeChannel is currently an array index; keying off ch.key works better once entries live in two different places in the UI.
  • When a dropdown entry is selected, reflect it in the trigger (More ⌄ becomes MacPorts ⌄, with the active underline). Otherwise the selection goes invisible and the command row looks like it changed on its own.
  • Dismiss on outside click and Escape; aria-haspopup="menu" and aria-expanded on the trigger, role="menuitem" on the entries.
  • Anchor the panel with right: 0 so it grows leftward — left: 0 overflows the viewport on mobile.
  • The install block's maxWidth: 460 needs a bump (520 works). The curl one-liner is ~406px at 13px Menlo and gets ellipsised otherwise.
  • A divider plus an All install options → link to /docs/installation at the foot of the panel gives the release binaries and build-from-source a place to live too.
  • New keys in all four of en/zh/ja/rucheck-translation-sync will fail on a missing one.

Don't feel obliged if this is more than you signed up for — say the word and I'll pick it up, and land your icon and channel entry on top. Either way the MacPorts entry is going in.

And genuinely: thank you for maintaining the port, and I hope you'll keep it up. You having v1.9.3 submitted within a couple of days of the tag is a better answer to my "will it go stale" worry than anything I could have argued. We'll link to it from the install docs as well.

@wu21-web

Copy link
Copy Markdown
Contributor Author

That is quite thoughtful of you. I will get this done today.

@wu21-web
wu21-web marked this pull request as draft August 14, 2026 05:26
@wu21-web

Copy link
Copy Markdown
Contributor Author

I won't really add "Scripts" tab to there for now, because I consider this as a job for @amh1k . The related pull request for this is #797 .

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

I won't really add "Scripts" tab to there for now, because I consider this as a job for @amh1k . The related pull request for this is #797 .

@wu21-web You only need to focus on more -> MacPorts

@wu21-web

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 How is this:

Screen.Recording.2026-08-14.at.3.00.28.PM.mp4

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

@lizhengfeng101 How is this:

Screen.Recording.2026-08-14.at.3.00.28.PM.mp4

nice work!

@wu21-web
wu21-web marked this pull request as ready for review August 14, 2026 06:16
@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Nice — you got there faster than I expected, and pulling the shape into an InstallChannel interface is better than what I had sketched. Two corrections to my last comment, one of which is me walking back bad advice.

First: I am not asking you to build keyboard navigation. I'm asking for less than what's there.

I told you to add role="menu" and role="menuitem". That was wrong, and I'm sorry for sending you down that path. Those roles are a promise to assistive technology that arrow keys, Home/End and focus cycling all work — the full WAI-ARIA menu widget. We don't want to build that for an install-command switcher, and declaring the role without implementing it is worse than never declaring it: a screen reader announces "menu", the user presses , and nothing happens.

The right pattern here is a plain disclosure, which only requires aria-expanded. Native <button> elements already give you Tab and Enter for free, and your Escape handler is already good. So the fix is to delete three attributes, not to write a keyboard handler:

   <button
     type="button"
-    aria-haspopup="menu"
     aria-expanded={menuOpen}
+    aria-controls="install-more-panel"
     onClick={() => setMenuOpen((open) => !open)}

-  <div role="menu" style={{ ... }}>
+  <div id="install-more-panel" style={{ ... }}>

-      <button type="button" role="menuitem" onClick={...}>
+      <button type="button" onClick={...}>

Second: the thing I flagged is a state bug, not a missing keyboard feature.

The primary-tab onClick sets the channel but never closes the panel:

-  onClick={() => setActiveChannelKey(ch.key)}
+  onClick={() => { setActiveChannelKey(ch.key); setMenuOpen(false); }}

With a mouse this already appears to work — but only because your document-level mousedown listener happens to fire before click. It's accidentally correct. Keyboard activation dispatches click with no preceding mousedown, so:

panel open → Tab to Homebrew → Enter
→ command row switches to `brew install …`
→ trigger label reverts to "More"
→ panel is still hanging open, listing MacPorts

Adding that one line fixes the keyboard case and makes the mouse case intentional instead of lucky.

Two smaller ones while you're in there:

  • The outside-click effect only listens for mousedown. iOS Safari doesn't reliably synthesise mouse events for taps on non-interactive elements, so add touchstart to the same handler (and widen the param to MouseEvent | TouchEvent).
  • useState('npm') hardcodes a key that's declared right above it — useState(INSTALL_CHANNELS[0].key) won't drift when the list is reordered.

Could you add tests for this?

HeroSection has no test file yet, and this PR is what turns it from static markup into a small state machine — open, close, select, dismiss. There's already @testing-library/react and user-event in the project, plus ErrorBoundary.test.tsx as a precedent.

I wrote these while checking the bug above, so they're yours — drop them in as pages/src/components/HeroSection.test.tsx:

// SPDX-License-Identifier: Apache-2.0
// Copyright 2026 alibaba/open-code-review Contributors

import { describe, it, expect } from 'vitest';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { MemoryRouter } from 'react-router-dom';
import { LanguageProvider } from '../i18n';
import HeroSection from './HeroSection';

function renderHero() {
  render(
    <MemoryRouter>
      <LanguageProvider>
        <HeroSection />
      </LanguageProvider>
    </MemoryRouter>,
  );
}

// The panel is found through the id that `aria-controls` already points at, so
// the test leans on the accessibility wiring instead of a test-only hook.
const panel = () => document.getElementById('install-more-panel');
const trigger = () => screen.getByRole('button', { name: /More|MacPorts/i });

describe('HeroSection install channels', () => {
  it('starts on the first channel with the panel closed', () => {
    renderHero();
    expect(screen.getByText('npm i -g @alibaba-group/open-code-review')).toBeTruthy();
    expect(panel()).toBeNull();
  });

  it('picking an overflow channel swaps the command and closes the panel', async () => {
    const user = userEvent.setup();
    renderHero();

    await user.click(trigger());
    expect(panel()).not.toBeNull();

    await user.click(screen.getByRole('button', { name: /MacPorts/i }));

    expect(screen.getByText('sudo port install open-code-review')).toBeTruthy();
    expect(panel()).toBeNull();
  });

  it('closes when a primary tab is clicked', async () => {
    const user = userEvent.setup();
    renderHero();

    await user.click(trigger());
    await user.click(screen.getByRole('button', { name: /Homebrew/i }));

    expect(screen.getByText('brew install open-code-review')).toBeTruthy();
    expect(panel()).toBeNull();
  });

  // Keyboard activation dispatches `click` with no preceding `mousedown`, so
  // this does not go through the same path as the test above.
  it('closes when a primary tab is activated by keyboard', async () => {
    const user = userEvent.setup();
    renderHero();

    await user.click(trigger());
    screen.getByRole('button', { name: /Homebrew/i }).focus();
    await user.keyboard('{Enter}');

    expect(screen.getByText('brew install open-code-review')).toBeTruthy();
    expect(panel()).toBeNull();
  });

  it('closes on Escape and on an outside click', async () => {
    const user = userEvent.setup();
    renderHero();

    await user.click(trigger());
    await user.keyboard('{Escape}');
    expect(panel()).toBeNull();

    await user.click(trigger());
    await user.click(document.body);
    expect(panel()).toBeNull();
  });

  it('reflects the selected overflow channel on the trigger', async () => {
    const user = userEvent.setup();
    renderHero();
    expect(screen.getByRole('button', { name: /^More$/i })).toBeTruthy();

    await user.click(trigger());
    await user.click(screen.getByRole('button', { name: /MacPorts/i }));

    const collapsed = screen.getByRole('button', { name: /MacPorts/i });
    expect(collapsed.getAttribute('aria-expanded')).toBe('false');
    expect(screen.queryByRole('button', { name: /^More$/i })).toBeNull();
  });
});

I ran these against your HEAD with the four fixes applied: typecheck clean, lint no errors, 25 tests passing across 6 files. Then I reverted just the one-line setMenuOpen(false) fix and only closes when a primary tab is activated by keyboard went red — so the suite does catch the regression rather than merely passing.

Two notes on the tests so nothing surprises you:

  • They deliberately use plain toBeTruthy() / toBeNull() instead of toBeInTheDocument(). @testing-library/jest-dom is loaded in vitest.setup.ts but its matcher types aren't wired into tsconfig.json, so the nicer matchers pass at runtime and fail npm run typecheck. Matches what ErrorBoundary.test.tsx does.
  • Rendering HeroSection in jsdom logs THREE.WebGLRenderer: Error creating WebGL context — that's ColorBends hitting a canvas jsdom doesn't implement, the ErrorBoundary swallows it, and the tests are unaffected. Noisy but harmless.

Smaller polish, take or leave

  • The trigger doesn't render the selected channel's icon, so the button visibly jumps width when you switch between "More" and "MacPorts". activeChannel.icons[0] would settle it.
  • The chevron doesn't rotate when the panel opens — transform: menuOpen ? 'rotate(180deg)' : 'none' is cheap feedback.
  • bug_report.yml says Macports; the brand spelling is MacPorts, which is what your i18n label and the SVG <title> both use.
  • INSTALL_CHANNELS is SCREAMING_CASE but primaryChannels/secondaryChannels next to it are camelCase.
  • The panel uses an opaque #1a1a1a, whereas the toast in this same file uses rgba(…) with backdropFilter: blur. Matching it would sit better against the shader background.
  • Worth a line in the PR description that maxWidth: 460 → 520 is headroom for the install-script command that's coming, otherwise the next reviewer will wonder why it grew with no long command in the diff.

Once the four fixes and the test file are in, this is good to merge from my side. Thanks for taking on more than you originally signed up for here.

@wu21-web
wu21-web force-pushed the mp branch 2 times, most recently from 8d1446e to cc3a882 Compare August 14, 2026 06:47

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lizhengfeng101
lizhengfeng101 merged commit d59edda into alibaba:main Aug 14, 2026
12 checks passed
@wu21-web
wu21-web deleted the mp branch August 14, 2026 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants