-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add custom agent to migrate code from v9 to v8 #35599
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
Open
Anush2303
wants to merge
1
commit into
microsoft:master
Choose a base branch
from
Anush2303:usr/agupta/v9tov8MigrationAgent
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
119 changes: 119 additions & 0 deletions
119
packages/charts/.github/agents/v9tov8Migration.agent.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| --- | ||
| description: 'Migrate code from functional component(v9) to class component(v8) in accordance with Fluent UI standards.' | ||
| tools: ['vscode', 'execute', 'read', 'edit', 'search', 'web', 'agent', 'todo'] | ||
| --- | ||
|
|
||
| **Difference between v8 and v9:** | ||
|
|
||
| - **v8**: | ||
|
|
||
| 1. class component code written inside `packages/charts/react-charting`. | ||
| 2. Traditional component lifecycle methods (e.g., componentDidMount, componentDidUpdate). | ||
| 3. State is managed using this.state and this.setState. | ||
| 4. Props accessed via this.props. | ||
|
|
||
| - **v9**: | ||
| 1. functional component code written inside `packages/charts/react-charts`. | ||
| 2. Utilizes React hooks (e.g., useState, useEffect) for state and lifecycle management. | ||
| 3. State is managed using useState hook. | ||
| 4. Direct props destructuring. | ||
|
|
||
| **Difference between v8 styles file and v9 styles file:** | ||
|
|
||
| - **example v8 styles file:** | ||
| `import { FontWeights } from '@fluentui/react/lib/Styling'; | ||
| import { IHeatMapChartStyleProps, IHeatMapChartStyles } from './HeatMapChart.types'; | ||
| export const getHeatMapChartStyles = (props: IHeatMapChartStyleProps): IHeatMapChartStyles => { | ||
| const { theme } = props; | ||
| return { | ||
| root: {}, | ||
| text: [ | ||
| theme.fonts.medium, | ||
| { | ||
| pointerEvents: 'none', | ||
| fontWeight: FontWeights.semibold, | ||
| }, | ||
| ], | ||
| subComponentStyles: {}, | ||
| }; | ||
| };` | ||
|
|
||
| - **_Some important points to note:_** | ||
|
|
||
| - Uses mergeStyleSets from @fluentui/react/lib/Styling | ||
| - Exports a function that returns styles (e.g., getHeatMapChartStyles) | ||
| - Uses theme-based styling with theme.fonts, theme.palette | ||
| - Styles are defined as objects or arrays of style rules | ||
| - Interfaces have I prefix (e.g., IHeatMapChartStyleProps, IHeatMapChartStyles) | ||
|
|
||
| - **example v9 styles file:** | ||
| `'use client'; | ||
| import { makeStyles, mergeClasses } from '@griffel/react'; | ||
| import type { HeatMapChartProps, HeatMapChartStyles } from './HeatMapChart.types'; | ||
| import type { SlotClassNames } from '@fluentui/react-utilities'; | ||
| import { typographyStyles } from '@fluentui/react-theme'; | ||
|
|
||
| export const heatmapChartClassNames: SlotClassNames<HeatMapChartStyles> = { | ||
| root: 'fui-hmc**root', | ||
| text: 'fui-hmc**text', | ||
| calloutContentRoot: 'fui-hmc\_\_calloutContentRoot', | ||
| xAxis: '', | ||
| yAxis: '', | ||
| legendContainer: '', | ||
| hover: '', | ||
| descriptionMessage: '', | ||
| tooltip: '', | ||
| axisTitle: '', | ||
| chartTitle: '', | ||
| opacityChangeOnHover: '', | ||
| shapeStyles: '', | ||
| chartWrapper: '', | ||
| svgTooltip: '', | ||
| chart: '', | ||
| axisAnnotation: '', | ||
| plotContainer: '', | ||
| annotationLayer: '', | ||
| }; | ||
| const useStyles = makeStyles({ | ||
| root: {}, | ||
| text: { | ||
| ...typographyStyles.body1Strong, | ||
| pointerEvents: 'none', | ||
| }, | ||
| calloutContentRoot: { | ||
| maxWidth: '238px', | ||
| }, | ||
| }); | ||
|
|
||
| export const useHeatMapChartStyles = (props: HeatMapChartProps): HeatMapChartStyles => { | ||
| const baseStyles = useStyles(); | ||
|
|
||
| return { | ||
| root: mergeClasses(heatmapChartClassNames.root, baseStyles.root /_, props.styles?.root_/), | ||
| text: mergeClasses(heatmapChartClassNames.text, baseStyles.text /_, props.styles?.text_/), | ||
| calloutContentRoot: mergeClasses( | ||
| heatmapChartClassNames.calloutContentRoot, | ||
| baseStyles.calloutContentRoot /_, props.styles?.calloutContentRoot_/, | ||
| ), | ||
| }; | ||
| };` | ||
|
|
||
| - **_Some important points to note:_** | ||
|
|
||
| - Uses makeStyles and mergeClasses from @griffel/react | ||
| - Exports a hook function that returns styles (e.g., useHeatMapChartStyles) | ||
| - Uses typographyStyles from @fluentui/react-theme for text styles | ||
| - Styles are defined as objects within makeStyles | ||
| - Interfaces do not have I prefix (e.g., HeatMapChartProps, HeatMapChartStyles) | ||
|
|
||
| - Migrate the provided v9 functional component code to a v8 class component code, ensuring to replace hooks with appropriate lifecycle methods and state management. | ||
|
|
||
| - Follow all the migration steps mentioned below to ensure a successful migration. | ||
|
|
||
| - **Migration steps:** | ||
| 1. Take the commit id from the user which needs to be migrated from v9 to v8. | ||
| 2. use git commands to fetch the file content from that commit. | ||
| 3. Use the above differences and points to convert the functional component code to class component code. | ||
| 4. Ensure all changes of that particular commit are addressed in the migration. **_ONLY MAKE CODE CHANGES FROM THAT COMMIT. DO NOT MAKE ANY OTHER CHANGES._** | ||
| 5. **_IMPORTANT:_** After migration, run `yarn build` to build the `react-charting` package and ensure there are no build errors. | ||
| 6. **_IMPORTANT:_** Then run `npx cross-env TZ=UTC jest` to ensure all tests are passing. If there are snapshots failures, update the snapshots by running `npx cross-env TZ=UTC jest -u`. | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
AFAIK this should live in
.github/agents/rather nested within charts folder ?lets fix it and use some common pattern to be able to easily convey the meaning , eg
agents/charts.v9-to-v8-migration.agent.mdThere 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.
we want to keep it specific to charts package only