Skip to content

Conversation

@danawan0409
Copy link

@danawan0409 danawan0409 commented Nov 30, 2025

SUMMARY

To migrate the Column component, the following steps were performed:

  • Renamed the index file from index.jsindex.ts
  • Renamed the component file from Column.jsxColumn.tsx
  • Made the following type changes in Column.tsx:
    • Replaced the propTypes constant with an type ColumnProps`
    • Typed callbacks and functions to their expected values
  • Renamed the test file from Column.test.jsxColumn.test.tsx
  • Added the following type changes in Column.test.tsx:
    • Created interface ColumnTestProps mirroring ColumnProps from Column.tsx
  • Renamed the test file from NewColumn.jsxNewColumn.tsx
  • Renamed the test file from NewColumn.test.jsxNewColumn.test.tsx
  • Addressed all ESLint warnings and TypeScript errors

TESTING INSTRUCTIONS

CI and unit tests. Add a Column layout element to a dashboard and confirm that it renders as expected.

ADDITIONAL INFORMATION

  • Has associated issue: [SIP-36] Proposal for standardizing use of TypeScript [SIP-36] Proposal for standardizing use of TypeScript #9101
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@rusackas @msyavuz @justinpark @sadpandajoe @geido

@bito-code-review
Copy link
Contributor

bito-code-review bot commented Nov 30, 2025

Bito Review Skipped - No Changes Detected

Bito didn't review this pull request because we did not detect any changes in the pull request to review.

@dosubot dosubot bot added change:frontend Requires changing the frontend dashboard:editmode Related to te Dashboard edit mode labels Nov 30, 2025
@danawan0409 danawan0409 changed the title Refactor/column chore(ts): Migrate Header.jsx to Header.tsx Nov 30, 2025
@danawan0409 danawan0409 changed the title chore(ts): Migrate Header.jsx to Header.tsx chore(ts): Migrate Column.jsx to Column.tsx Nov 30, 2025
@rusackas rusackas requested a review from Copilot December 1, 2025 19:28
Copilot finished reviewing on behalf of rusackas December 1, 2025 19:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the Column dashboard component from JavaScript/JSX to TypeScript/TSX as part of the broader TypeScript standardization effort (SIP-36). The migration converts PropTypes to TypeScript interfaces, adds proper type annotations to callbacks and functions, and updates corresponding test files with appropriate type definitions.

Key changes:

  • Replaced PropTypes with TypeScript type definitions for component props
  • Added type annotations for callbacks (resize handlers, change handlers)
  • Updated test files with proper TypeScript types for mocks and test props

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
NewColumn.tsx Converted from named function to arrow function with FC type, added FC import
NewColumn.test.tsx Added TypeScript types to mock component parameters
Column/index.ts Simplified export statement to use inline default export
Column.tsx Migrated from PropTypes to TypeScript type definitions, added proper callback types from re-resizable library, added heightMultiple prop to ResizableContainer
Column.test.tsx Added TypeScript interfaces for test props, typed all mock implementations, updated attribute names from prop attributes to data attributes
Comments suppressed due to low confidence (3)

superset-frontend/src/dashboard/components/gridComponents/Column/Column.test.tsx:226

  • The test assertion references columnWithoutChildren.meta.width but the test calls setup() with no arguments, which means it uses props.component (i.e., mockLayout.present.COLUMN_ID), not columnWithoutChildren. This test should either:
  1. Pass { component: columnWithoutChildren } to the setup() call on line 224, OR
  2. Change line 226 to check against props.component.meta.width instead of columnWithoutChildren.meta.width

Currently, the test happens to pass because both have the same width value (4), but this is misleading and could break if the fixture values change.
superset-frontend/src/dashboard/components/gridComponents/Column/Column.test.tsx:86

  • [nitpick] The comment "or whatever number you expect" is vague and suggests uncertainty. Since this value is set to match GRID_DEFAULT_CHART_WIDTH (which is 4), consider either removing the comment entirely or making it more specific, such as: width: 4, // GRID_DEFAULT_CHART_WIDTH
    superset-frontend/src/dashboard/components/gridComponents/Column/Column.tsx:47
  • Duplicate import of FC from 'react'. FC is imported on line 47 but should be included in the destructured import from 'react' on lines 19-26 instead. Remove the duplicate import on line 47 and add FC to the first import statement.

@sadpandajoe
Copy link
Member

@danawan0409 looks like there are a few errors, you might want to run npm run type to see if it resolves these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend dashboard:editmode Related to te Dashboard edit mode size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants