Skip to content

feat: Page Mobile Adaptation: /about Page + Modals#601

Open
alibabaedge wants to merge 2 commits intoupdates/dev-updatefrom
agent/issue-600
Open

feat: Page Mobile Adaptation: /about Page + Modals#601
alibabaedge wants to merge 2 commits intoupdates/dev-updatefrom
agent/issue-600

Conversation

@alibabaedge
Copy link
Copy Markdown
Member

What: Adapt the /about page and its two modals (Our Tools, Our Manifesto) for mobile and tablet viewports.

Why: Closes #600

How:

  • Made about page description full-width on mobile (w-full md:w-2/3)
  • Stacked modal trigger buttons vertically on mobile (flex-col with space-y-4, horizontal on md:)
  • Hidden decorative green-man image on mobile viewports
  • Added BaseModalMobile component for both modals on mobile (touch-friendly close button), using hidden md:block / block md:hidden pattern
  • Adjusted modal content padding for mobile (p-4 pt-3 vs desktop p-10 pt-5)
  • Fixed tab list horizontal overflow by switching from space-x-12 to gap-1 on mobile with overflow-x-auto
  • No localization changes needed (no new user-facing text added)
  • Desktop layout preserved (all changes use md: breakpoint prefix)

Testing:

  • ESLint: passes (no errors on changed files)
  • yarn build: passes with no type errors
  • Playwright screenshots taken at 375px mobile viewport width

Screenshot:

About page (mobile 375px):
about-mobile

Our Tools modal (mobile):
tools-modal

Our Manifesto modal (mobile):
manifesto-modal

- Make about page layout responsive: full-width description on mobile,
  vertically stacked modal buttons, hidden green-man image below md
- Add BaseModalMobile for Our Tools and Our Manifesto modals on mobile
  viewports using hidden/block md: pattern for desktop/mobile switching
- Fix tab list horizontal overflow by replacing space-x with smaller
  gap values and adding overflow-x-auto
Copy link
Copy Markdown
Contributor

@m1amgn m1amgn left a comment

Choose a reason for hiding this comment

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

AI Review Summary

Verdict: REQUEST CHANGES

[HIGH] Dual modal rendering causes useOnClickOutside conflict — modals unusable on mobile

Files: src/app/[locale]/about/modals/our-manifesto-modal.tsx, src/app/[locale]/about/modals/our-tools-modal.tsx

Both BaseModal (desktop) and BaseModalMobile are rendered simultaneously, toggled only by CSS visibility (hidden md:block / block md:hidden). Both components unconditionally register useOnClickOutside document-level mousedown listeners (usehooks-ts v3.1.0, line 527 of dist/index.js).

What happens on mobile:

  1. User opens modal → isToolsOpened = true → both modals mount with opened={true}
  2. Desktop modal is hidden via CSS (display: none from wrapper's hidden md:block), but its useOnClickOutside listener is still active on document
  3. User clicks/taps anywhere on the visible mobile modal content
  4. Desktop modal's listener runs: desktopRef.current.contains(event.target)false (click target is in the mobile modal, not the desktop modal's DOM subtree)
  5. onClose() fires → setIsToolsOpened(false)modal immediately closes

The useOnClickOutside implementation has no visibility check — it only checks target.isConnected (true for all DOM-connected nodes, including hidden ones) and ref.current.contains(target).

Confidence: 92%

Fix: Use conditional rendering instead of CSS show/hide so only one modal mounts at a time. For example, use a useMediaQuery hook:

// Simplified approach — only mount one modal
const isMobile = useMediaQuery('(max-width: 767px)');
const ModalComponent = isMobile ? BaseModalMobile : BaseModal;

Or guard each modal with the opened state to prevent mounting the hidden one:

{isToolsOpened && (
  <div className="hidden md:block">
    <BaseModal ...>{...}</BaseModal>
  </div>
)}

This prevents the hidden modal's useOnClickOutside from registering when not needed. The same pattern is already used safely elsewhere in the codebase (see validator-list-item-links-mobile.tsx:31 which conditionally renders BaseModalMobile).


Other changes look good: responsive spacing on the about page, full-width description on mobile, stacked buttons layout, hidden green-man image, and tab-list gap/overflow changes are all clean mobile adaptations.

Use conditional rendering instead of CSS show/hide for desktop/mobile
modals to prevent useOnClickOutside listener conflict. When both
BaseModal and BaseModalMobile were mounted simultaneously (toggled
only by CSS visibility), the hidden modal's click-outside listener
would fire on clicks in the visible modal, immediately closing it.

Now modals only mount when isToolsOpened is true, matching the
existing pattern in validator-list-item-links-mobile.tsx.
Copy link
Copy Markdown
Contributor

@m1amgn m1amgn left a comment

Choose a reason for hiding this comment

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

AI Review Summary

Verdict: APPROVE

This PR adds mobile responsiveness to the /about page — layout, modals, and tab list. The changes are clean and straightforward.

What was reviewed:

  • about/page.tsx — responsive layout: full-width text on mobile, stacked buttons, hidden green-man image below md
  • our-manifesto-modal.tsx / our-tools-modal.tsx — conditional rendering of BaseModal (desktop) and BaseModalMobile (mobile) using hidden md:block / block md:hidden pattern
  • tab-list.tsx — switched from space-x-* to gap-* with overflow-x-auto for better mobile tab scrolling
  • Verified BaseModalMobile exists at src/app/[locale]/components/common/modal/base-modal-mobile.tsx (pre-existing component)
  • Verified all import paths resolve correctly per tsconfig.json aliases
  • No new user-facing strings introduced (reuses existing translations) — locale file updates not required

No issues found above the confidence threshold. The responsive CSS patterns are correct, the component splitting is sound, and there are no bugs, security concerns, or architecture violations.

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