Skip to content
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

feat: a new chart type - Box Chart #1382

Merged
merged 40 commits into from
Aug 25, 2020
Merged

feat: a new chart type - Box Chart #1382

merged 40 commits into from
Aug 25, 2020

Conversation

BBSQQ
Copy link
Member

@BBSQQ BBSQQ commented Aug 2, 2020

new chart type: Box

  • 支持 grouped box chart, outliers
  • 增加测试用例覆盖 legend, tooltip 等
  • 暂不支持 transpose 直角坐标和极坐标

@BBSQQ BBSQQ requested review from lessmost, hustcc and visiky August 2, 2020 10:06
@auto-add-label auto-add-label bot added the WIP label Aug 2, 2020
Copy link

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

Could you please add tests to make sure this change works as expected?

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2020

This pull request introduces 3 alerts when merging c4a613d into 4e2c047 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@hustcc
Copy link
Member

hustcc commented Aug 3, 2020

箱线图会做成什么样子,可以先简单描述吗?

@lgtm-com
Copy link

lgtm-com bot commented Aug 3, 2020

This pull request introduces 3 alerts when merging bbaec16 into a5f900c - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@BBSQQ
Copy link
Member Author

BBSQQ commented Aug 3, 2020

箱线图会做成什么样子,可以先简单描述吗?

实现 g2 所有 demo
image

export interface BoxOptions extends Options {

  /** 分类字段 */
  readonly catField: string;

  /** 测量字段映射 box range [low, q1, median, q3, hight] 五个字段 */
  readonly measureField: [string?, string?, string?, string?, string?];

  /** 颜色字段 */
  readonly colorField: string;

  /** 柱子样式配置,可选 */
  readonly boxStyle?: ShapeStyle | ((x: any, y: any, color?: any) => ShapeStyle);

  /** 是否分组箱线图 默认 false*/
  readonly isGroup?: boolean;

  /** 分组拆分字段 默认 false*/
  readonly groupField?: string;

  /** outliers 字段 */
  readonly outliersField: string;
  
  /** 在直角坐标系还是极坐标系 默认 rect 直角坐标系*/
  readonly coord: 'rect' | 'polar'; 
}

// 传入数据格式
interface BoxData {
  x: string
  low?: number
  q1?: number
  median?: number
  q3?: number
  high?: number
  outliers?: number[] | number
}

处理边界:

  • 检测传入数据类型是否符合期望数据类型,如果是 null 处理成空
  • outliers 如果为数组则显示多个点,如果为 number 则显示一个点
  • measureField 默认从 0 开始,因为箱线图一般用于科学统计场景
  • 其他 tooltips animation legend 等,之后看情况再处理

height: 500,
data: boxData,
xField: 'x',
yField: ['low', 'q1', 'median', 'q3', 'high'],
Copy link
Member

Choose a reason for hiding this comment

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

是不是相当于 YField 一定要传 5 个参数,且严格按顺序(如果说缺少一个字段参数,会绘制不出来吗?比如说不需要展示中位线,是否可以不要第三个参数 median?又或者说 中间三个参数是否可以在内部用 data-set 计算就行了)

Copy link
Member Author

Choose a reason for hiding this comment

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

是不是相当于 YField 一定要传 5 个参数,且严格按顺序(如果说缺少一个字段参数,会绘制不出来吗?比如说不需要展示中位线,是否可以不要第三个参数 median?又或者说 中间三个参数是否可以在内部用 data-set 计算就行了)

  1. 需要按照顺序传5个参数
  2. 倒是不需要都传,不穿也可以绘制出来;
  3. median 不是由 q1 q3 或者 low high 决定的,而是由原数据决定的,http://tuzhidian.com/chart?id=5c666f91372bb033b9c2fa75
  4. 可以让用户只传原数据,我们ds处理,但是会不会数据量大,我看 g2 demo 是这样指定的

Copy link
Member

Choose a reason for hiding this comment

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

5 个数据相当于是统计计算之后出来。如果按照直方图的套路,确实应该内置统计。

但是确实会增加数据量,而且数据理解成本可能更高了,我的想法是可以按照这样的方式,直接传五个字段。

src/plots/box/adaptor.ts Outdated Show resolved Hide resolved
src/plots/box/adaptor.ts Outdated Show resolved Hide resolved
src/plots/box/adaptor.ts Outdated Show resolved Hide resolved
src/plots/box/adaptor.ts Outdated Show resolved Hide resolved
src/plots/box/index.ts Outdated Show resolved Hide resolved
@hustcc
Copy link
Member

hustcc commented Aug 13, 2020

outliersField
  1. catField 还是叫 xField 吧
  2. colorField 和 groupField 是一个含义吧
  3. coord 用全名 coordinate,要不要传完整 coordinate 结构? { type: '', cfg: {} };

@hustcc hustcc changed the base branch from v2 to master August 14, 2020 07:48
@BBSQQ BBSQQ changed the title WIP: Box feat: a new chart type - Box Chart Aug 25, 2020
@auto-add-label auto-add-label bot added enhancement New feature or request and removed WIP labels Aug 25, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2020

This pull request introduces 1 alert when merging b091805 into eacf58f - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

{ value: 56 },
{ value: 67 }
];
const data = [{ value: 20 }, { value: 34 }, { value: 56 }, { value: 67 }];
Copy link
Member

Choose a reason for hiding this comment

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

这个 diff 可以去掉。

@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2020

This pull request introduces 1 alert when merging bb4b134 into eacf58f - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

| content | _boolean_, _object_ | 指标卡内容 |
|     formatter | _function_ | 通过回调的方式,设置指标卡内容 |
|     style | _object_ | 指标卡内容样式 |
Copy link
Member

Choose a reason for hiding this comment

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

同上。

| content | _boolean_, _object_ | 指标卡内容 |
|     formatter | _function_ | 通过回调的方式,设置指标卡内容 |
|     style | _object_ | 指标卡内容样式 |
Copy link
Member

Choose a reason for hiding this comment

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

同上。

src/plots/box/index.ts Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2020

This pull request introduces 1 alert when merging 465f938 into eacf58f - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2020

This pull request introduces 1 alert when merging 9c1e077 into eacf58f - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

import { BoxOptions } from './types';
import { flow, pick } from '../../utils';
import { AXIS_META_CONFIG_KEYS } from '../../constant';
import * as constant from './const';
Copy link
Member

Choose a reason for hiding this comment

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

import { Range, xxx }

建议不使用 import * as yyy,因为这个就存在一个命名问题,不同人命名风格又不一样。。

Copy link
Member

Choose a reason for hiding this comment

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

文件名就叫 constant.ts 吧,不用简写~


// make yField and outliersField share y mate
if (outliersField) {
const syncName = '$$y_outliers$$';
Copy link
Member

Choose a reason for hiding this comment

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

常量,放到 constant.ts 中

@@ -0,0 +1,2 @@
export const BOX_RANGE = '$$range$$';
export const BOX_RANGE_ALIAS = '区间信息';
Copy link
Member

@hustcc hustcc Aug 25, 2020

Choose a reason for hiding this comment

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

nit: 默认的名字用英文不?

import { BoxOptions } from './types';
import { adaptor } from './adaptor';
import { deepMix } from '@antv/util';
import * as constant from './const';
Copy link
Member

Choose a reason for hiding this comment

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

import { BOX_RANGE_ALIAS } from './constant';

/** 分组拆分字段 */
readonly groupField?: string;
/** 颜色字段,可选 */
readonly colorField?: string;
Copy link
Member

@hustcc hustcc Aug 25, 2020

Choose a reason for hiding this comment

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

是否有必要三个字段 @me-momo @zqLu 这里用那个一比较合适?

@hustcc hustcc merged commit c660d8f into master Aug 25, 2020
@hustcc hustcc deleted the v2-box branch August 25, 2020 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PR: merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants