Skip to content

Comments

Refactor SelectExportTypeModal with OS-Legal UI and styled-components#925

Merged
JSv4 merged 1 commit intomainfrom
claude/refactor-export-modal-style-NjIEB
Feb 22, 2026
Merged

Refactor SelectExportTypeModal with OS-Legal UI and styled-components#925
JSv4 merged 1 commit intomainfrom
claude/refactor-export-modal-style-NjIEB

Conversation

@JSv4
Copy link
Collaborator

@JSv4 JSv4 commented Feb 22, 2026

Summary

Refactored the SelectExportTypeModal component to use the OS-Legal design system and styled-components, replacing Semantic UI components with custom-styled alternatives. The modal now features improved visual design, better mobile responsiveness, and cleaner component architecture.

Key Changes

  • UI Framework Migration: Replaced Semantic UI modal components (Modal, Modal.Header, Modal.Content, Modal.Actions) with OS-Legal UI components (Modal, ModalHeader, ModalBody, ModalFooter)
  • Styling Overhaul: Converted inline styles and CSS objects to styled-components with CSS variables for theming and consistent spacing
  • Component Props: Updated component interface from { visible: boolean } to { open: boolean; onClose: () => void } for better semantic clarity and control
  • Export Format Selection: Replaced dropdown-based format selection with interactive card-based UI featuring icons, descriptions, and visual selection indicators
  • Visual Enhancements:
    • Added lucide-react icons for section headers and format options
    • Implemented custom radio-button style indicators for format selection
    • Added gradient backgrounds and improved hover states
    • Enhanced typography hierarchy with styled section titles
  • Mobile Responsiveness: Added comprehensive mobile breakpoints (768px tablet, 375px mobile) with:
    • Sticky footer buttons on mobile
    • Full-width button layout
    • Adjusted padding and spacing
    • Bottom sheet-style modal animation on small screens
  • Code Quality: Removed verbose comments, simplified state management, and improved callback naming (handleExportFormatChangehandleFormatSelect)
  • Testing: Added comprehensive Playwright component tests covering default state, format selection, post-processor handling, and mobile responsiveness with screenshot documentation

Notable Implementation Details

  • Styled-components use CSS custom properties (e.g., var(--oc-spacing-lg)) for consistent theming
  • Loading overlay uses absolute positioning with backdrop blur for non-intrusive feedback
  • Dropdown z-index explicitly set to prevent modal overlap issues
  • Mobile viewport uses safe-area-inset-bottom for notch-aware padding
  • Post-processor forms wrapped in styled cards with consistent spacing
  • Modal subtitle dynamically displays corpus name from reactive variable

https://claude.ai/code/session_01HDEJE3NBQ2XYN2bEuP3Yvh

Migrate the export type selection modal from Semantic UI to @os-legal/ui
component library, following the established modal pattern used in
CorpusModal and UploadModal.

Changes:
- Replace Semantic UI Modal/Button/Icon with @os-legal/ui Modal,
  ModalHeader, ModalBody, ModalFooter, Button, Spinner
- Replace Semantic UI icons with lucide-react (Package, FileOutput, etc.)
- Add styled-components with OS-Legal design tokens (CSS variables,
  form sections, responsive layout)
- Replace dropdown-based format selection with visual radio card UI
- Add mobile-responsive bottom-sheet layout with safe-area support
- Change component API from `visible` prop to `open`/`onClose` props
- Update Corpuses.tsx to use new prop API
- Add Playwright component test with 8 test cases covering default
  state, format switching, actions, post-processors, and mobile layout
- Add docScreenshot captures for export documentation
- Update export-import docs with step-by-step export instructions
  and auto-generated screenshot references

Closes #923

https://claude.ai/code/session_01HDEJE3NBQ2XYN2bEuP3Yvh
@claude
Copy link

claude bot commented Feb 22, 2026

Code Review

Overall this is a solid refactor — the card-based format selection is much cleaner than the old dropdown-inside-an-info-box pattern, the prop interface is semantically better, and the mobile responsiveness is a real improvement. A few things to address:


Issues

1. Local magic number: TABLET_BREAKPOINT = 768

SelectExportTypeModal.tsx adds a local constant:

const TABLET_BREAKPOINT = 768;

Per the project conventions in CLAUDE.md, magic numbers belong in the constants file. MOBILE_VIEW_BREAKPOINT is correctly imported from frontend/src/assets/configurations/constants.ts, but TABLET_BREAKPOINT is not. Either add it to the constants file and import it, or (if a tablet breakpoint already exists there under a different name) reuse it.


2. State not reset when modal reopens

When the modal is closed and reopened, exportFormat, selectedPostProcessors, postProcessorKwargs, and availablePostProcessors all retain their previous values. A user who picks FUNSD + a post-processor, cancels, then reopens the modal will see those selections still active.

Consider resetting state when open transitions from false to true:

useEffect(() => {
  if (open) {
    setExportFormat(ExportTypes.OPEN_CONTRACTS);
    setSelectedPostProcessors([]);
    setPostProcessorKwargs({});
    fetchPostProcessors();
  }
}, [open, fetchPostProcessors]);

3. closeOnDimmerClick behaviour change

The old modal had closeOnDimmerClick={false} — intentionally preventing accidental dismissal mid-configuration. The new modal inherits the OS-Legal Modal's default for this prop. Confirm the new Modal component also prevents dimmer-click dismissal, or explicitly pass the equivalent prop if needed.


4. Tests use waitForTimeout instead of element-based waits

From CLAUDE.md: "Wait for visible evidence, not just network-idle."

Both post-processor tests do:

await page.waitForTimeout(500);
await expect(page.locator("text=Select post-processors...")).toBeVisible();

A hardcoded 500 ms delay is fragile on slow CI. Replace with:

await expect(page.locator("text=Select post-processors...")).toBeVisible({ timeout: 5000 });

That waits for the element rather than sleeping.


5. FUNSD selection test does not assert the selected state

should allow switching to FUNSD format clicks the option and takes a screenshot but has no assertion that the selection actually changed (e.g., the SelectedIndicator is visible, or the option's border style changed). A screenshot alone is documentation, not a test. Add an explicit check, for example:

await funsdOption.click();
const funsdIndicator = page.locator('[data-testid="format-funsd"]').locator('.selected-indicator');
await expect(funsdIndicator).toBeVisible();

(Adjust selector to match the actual rendered DOM.)


Nits

  • z-index: 1000 !important in StyledModalWrapper for the Semantic UI dropdown menu is necessary here but worth a short comment explaining why (prevents the dropdown from clipping behind the modal overlay).
  • Hybrid Semantic UI / OS-Legal: The post-processor Dropdown is still Semantic UI while the rest of the modal is OS-Legal. Noted in the PR description as intentional; just flagging it should be tracked as follow-up to avoid long-term inconsistency.
  • Inline arrow wrappers: onClick={() => handleFormatSelect(ExportTypes.OPEN_CONTRACTS)} creates a new reference each render. Not a problem since FormatOption is a plain styled-component (no React.memo), but worth knowing if this component is ever profiled.

The documentation updates and the decision to always render the modal controlled by open (rather than conditional rendering in the parent) are both improvements. Good work on the test coverage overall — the gaps above are straightforward to fix.

@codecov
Copy link

codecov bot commented Feb 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@JSv4 JSv4 merged commit a95d9b6 into main Feb 22, 2026
14 checks passed
@JSv4 JSv4 deleted the claude/refactor-export-modal-style-NjIEB branch February 22, 2026 14:27
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