-
Notifications
You must be signed in to change notification settings - Fork 363
chore(chart bullet): convert to typescript #11756
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
chore(chart bullet): convert to typescript #11756
Conversation
@thatblindgeye since there are 17 examples, this PR will be big. Let me know if you want me to break it down into 2 PRs. Although the changes made will be simple. |
Preview: https://patternfly-react-pr-11756.surge.sh A11y report: https://patternfly-react-pr-11756-a11y.surge.sh |
@Mash707 let's try splitting it into 2 PRs and see how it looks. Even though it should be pretty straight forward conversions, it'll end up being a lot of lines touched so splitting it may make it a tad easier to review. |
@thatblindgeye I have included 9 examples in this PR, for the remaining I'll create a new PR and link it to this one. |
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.
Just some nits for file names/exports below, let me know what you think
``` | ||
|
||
### Primary measure outside range | ||
|
||
This is a yellow bullet chart with primary measure greater than max range. | ||
|
||
```js | ||
import { ChartBullet, ChartThemeColor } from '@patternfly/react-charts/victory'; | ||
```ts file = "ChartBulletPrimaryRange.tsx" |
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.
Nit: This file could maybe be "ChartBulletOutsideRange" instead
``` | ||
|
||
### Negative primary measure | ||
|
||
This bullet chart with negative primary measure is for measures considered to be bad when they are low. | ||
|
||
```js | ||
import { ChartBullet } from '@patternfly/react-charts/victory'; | ||
```ts file = "ChartBulletNegativePrimary.tsx" |
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.
Nit: maybe "ChartBulletNegativeMeasure"
``` | ||
|
||
### Reversed with right aligned legend | ||
|
||
This reversed bullet chart with right aligned legend is for measures considered to be good when they are low. | ||
|
||
```js | ||
import { ChartBullet } from '@patternfly/react-charts/victory'; | ||
```ts file = "ChartBulletReversedLegend.tsx" |
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.
Nit: we could drop the "Legend" in the name
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.
Ah good observation, since we don't any examples starting with "Reversed" dropping "Legend" makes it shorter
``` | ||
|
||
### Negative and positive primary measures | ||
|
||
This bullet chart with negative and positive primary measures has 4 legend items per row. | ||
|
||
```js | ||
import { ChartBullet } from '@patternfly/react-charts/victory'; | ||
```ts file = "ChartBulletNegativePositivePrimary.tsx" |
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.
```ts file = "ChartBulletNegativePositivePrimary.tsx" | |
```ts file = "ChartBulletNegativePositiveMeasure.tsx" |
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.
Was confused with the usage of "Primary" but looking at it now "Measure" makes more sense, aligns more with the example description
The suggestions look good to me, made the necessary changes. |
Towards #11719
The following examples of ChartArea will be converted to TypeScript:
Remaining examples are converted in #11758 to make it easier for reviewing