-
Notifications
You must be signed in to change notification settings - Fork 649
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
fix: attr映射逻辑优化 #1261
fix: attr映射逻辑优化 #1261
Conversation
This pull request fixes 1 alert when merging 9f7cea6 into b9753a2 - view on LGTM.com fixed alerts:
|
return { | ||
color: color[0], | ||
shape: shape && shape[0], | ||
size: size[0] |
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.
size 不要默认了吧,size 的默认值在theme 里配置,不然 interval 就没法区分是这里的默认还是主动设置的值
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.
去掉了
const { x, y, size = defaultSize } = normalized; | ||
const rect = convertRect({ x, y, size, y0 }); | ||
const { x, y } = normalized; | ||
const rect = convertRect({ x, y, y0, size: defaultSize, }); |
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.
这里的逻辑还有问题,如果有传size, 则要用传入的size 的,比如 <Interval size={10} />
packages/f2/src/controller/attr.ts
Outdated
// Unknown Attr type | ||
if (isNil(type)) { | ||
AttrConstructor = Category; | ||
delete attrOption.scale; |
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.
这个scale是需要的吧
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.
我理解不需要的,删除scale的目的是希望这个attr 通过数据自己创建scale
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.
创建scale 的开销会比较大,尤其是大数据量的时候,而且对于一个 chart 来说, scale 是唯一的,所以全局保留一份就好了
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.
比如有个字段是连续性数据,但是用这个字段来进行分类映射了,这时候就不能直接用数据上的scale
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.
能啊,scale 是用来标识数据的归一的方式,数据的归一方式是一样的呀
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.
比如这个例子,sales是数值字段,同时用来做了分类属性,就不能使用数据的scale了
<Chart data={data}>
<Axis field="year" />
<Axis field="sales" />
<Point
x="year"
y="sales"
size={12}
color={{
field: 'sales',
range: ['blue', 'red'],
}}
/>
</Chart>
而这里的代码https://github.com/antvis/F2/blob/v4.0.x/packages/f2/src/attr/base.ts#L18 判断了如果传入了scale,就使用scale,这样的话一个Category的Attr就在用一个Linear的Scale在做映射了
}); | ||
} | ||
return min + (max - min) * scale.scale(value); | ||
|
||
return interpolate(scale.scale(value)); |
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.
这里用d3-interpolate是为了支持颜色插值
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.
扩展 一个 color 之类的独立类型?总感觉这里直接用interpolate 有点奇怪
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.
嗯 可以的
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.
扩展一个color 的attr,感觉对外的概念有点奇怪
attrController.attrs = {}; | ||
this.attrs = attrController.getAttrs(); | ||
} | ||
|
||
_getAttrOptions(props) { |
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.
Attr处理的相关方法下沉到AttrController了,另外AttrController也要感知到 size、color、shape这些具体属性,方便构造attrOption
yMax, | ||
}; | ||
} | ||
|
||
function convertToPoints({ xMin, xMax, yMin, yMax }) { |
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.
这个文件后面想要干掉,感觉有点突兀
This pull request introduces 1 alert and fixes 3 when merging d3ce9a5 into 98474e2 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 3 when merging 6c2dfb2 into 98474e2 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 3 when merging 4e4c395 into 98474e2 - view on LGTM.com new alerts:
fixed alerts:
|
1. size/color/shape Attr 下沉到 AttrController 判断和处理,方便不同的attr类型生成不同的attrOption。
2. 支持分类属性进行线性映射(比如size/color,可以是线性映射)
3. createAttr时,Attr类型判断修改,兜底为Category,并且不使用数据的scale
Checklist
npm test
passesDescription of change