-
Notifications
You must be signed in to change notification settings - Fork 77
feat(FR-955): move Flex component to backend.ai-ui project #3640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/setup-bai-ui-pacakge
Are you sure you want to change the base?
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed that it works well. May we could add more examples for Flex but I guess it's enough now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
601e226
to
d28910b
Compare
5bfefd3
to
7d84ef1
Compare
d28910b
to
1128404
Compare
7d84ef1
to
6480f30
Compare
1128404
to
f0d6681
Compare
6480f30
to
9a877f7
Compare
f0d6681
to
d0dfb40
Compare
There was a problem hiding this 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 extracts the custom Flex
component into the backend.ai-ui
package, configures Storybook for UI component development, and updates its usage in the React app.
- Add and export the
Flex
component inbackend.ai-ui
- Configure Storybook (preview, main) and add stories for
Flex
- Update
StartPage.tsx
to importFlex
frombackend.ai-ui
instead of the local component
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
react/src/pages/StartPage.tsx | Swap local Flex import for the packaged version |
packages/backendai-ui/src/index.ts | Export Flex from package entry point |
packages/backendai-ui/src/components/Flex.tsx | New Flex component implementation |
packages/backendai-ui/src/components/Flex.stories.tsx | Storybook stories for Flex |
packages/backendai-ui/src/components/Button.tsx | Removed unused Button component |
packages/backendai-ui/package.json | Added Storybook scripts & dependencies |
packages/backendai-ui/.storybook/preview.ts | Storybook preview configuration |
packages/backendai-ui/.storybook/main.ts | Storybook main configuration |
packages/backendai-ui/.gitignore | Ignore Storybook log files |
Comments suppressed due to low confidence (1)
packages/backendai-ui/src/components/Flex.tsx:1
- There are currently no unit tests covering the
Flex
component’s prop handling (e.g., gap sizing or justify/align mapping). Adding tests for these variations would help ensure future changes don’t introduce regressions.
import { theme } from 'antd';
{ | ||
direction = 'row', | ||
wrap = 'nowrap', | ||
justify = 'flex-start', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default prop for justify
is set to 'flex-start'
, but the FlexProps
type expects one of 'start' | 'end' | 'center' | 'between' | 'around'
. Change it to 'start'
so the mapping logic applies correctly.
justify = 'flex-start', | |
justify = 'start', |
Copilot uses AI. Check for mistakes.
<div | ||
ref={ref} | ||
style={{ | ||
alignItems: 'stretch', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] There are duplicate style properties between the hard-coded base object (e.g., alignItems
, flexDirection
, display
) and the merged flexStyle
. Consider consolidating these into a single style object to reduce redundancy and simplify maintenance.
Copilot uses AI. Check for mistakes.
return typeof size === 'string' | ||
? // @ts-ignore | ||
token['padding' + size.toUpperCase()] | ||
: size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using @ts-ignore
can hide real typing issues. Consider refining the theme token type or using a type-safe lookup method instead of suppressing the error.
return typeof size === 'string' | |
? // @ts-ignore | |
token['padding' + size.toUpperCase()] | |
: size; | |
if (typeof size === 'string') { | |
const key = `padding${size.toUpperCase()}` as keyof typeof token; | |
return token[key] || 0; // Fallback to 0 if the key does not exist | |
} | |
return size; |
Copilot uses AI. Check for mistakes.
9a877f7
to
aae684f
Compare
d0dfb40
to
4526319
Compare
resolves #3618 (FR-955)
In this PR, move custom
Flex
component in react project tobackend.ai-ui
projectchanges
Flex
component tobackend.ai-ui
projectstorybook
Flex
componentChecklist: (if applicable)