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.

Sign up for GitHub

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

Jump to bottom

feat(word-cloud): imageMask选项支持url字符串格式 #1617

Merged
merged 5 commits into from
Sep 24, 2020
Merged

feat(word-cloud): imageMask选项支持url字符串格式 #1617

merged 5 commits into from
Sep 24, 2020

Conversation

zhangzhonghe
Copy link
Member

@auto-add-label auto-add-label bot added the enhancement New feature or request label Sep 22, 2020
@pr-triage pr-triage bot added the PR: draft label Sep 22, 2020
@zhangzhonghe zhangzhonghe marked this pull request as ready for review September 22, 2020 05:49
hustcc
hustcc reviewed Sep 22, 2020
View reviewed changes
src/plots/word-cloud/index.ts
this.options = {
...this.options,
imageMask: img || null,
};
Copy link
Member

Choose a reason for hiding this comment

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

这个可以在 adaptor 中处理吗？

Copy link
Member Author

Choose a reason for hiding this comment

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

1，现在adaptor中的代码都是同步运行的，而图片资源加载需要时间。
2，也可以这样，一开始就正常运行render，等到图片加载完成后，再更新配置选项，重新渲染。但我觉得这种效果不是很好。

Copy link
Member

Choose a reason for hiding this comment

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

效果主要取决于图表的加载时间，我觉得目前的方式也可以，但文档里面给个建议，URL 方式不要使用过大的图片。

Copy link
Member Author

Choose a reason for hiding this comment

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

目前来看，用base64效果最好。

@hustcc
Copy link
Member

hustcc commented Sep 22, 2020

增加一个 base64 的 demo。

@zhangzhonghe
Copy link
Member Author

增加一个 base64 的 demo。

OK

@zhangzhonghe
Copy link
Member Author

zhangzhonghe commented Sep 23, 2020 •
edited
Loading

@hustcc @lxfu1 目前有一个错误，就是当有图片遮罩，并且传给data-set的画布宽高为0时，会报错。这个最好在data-set中处理一下，我等下给data-set提个pr。

@zhangzhonghe zhangzhonghe mentioned this pull request Sep 23, 2020
Merged
hustcc
hustcc reviewed Sep 24, 2020
View reviewed changes
examples/word-cloud/basic/demo/image-mask.ts
height: 400,
wordField: 'name',
weightField: 'value',
imageMask: 'https://gw.alipayobjects.com/mdn/rms_2274c3/afts/img/A*07tdTIOmvlYAAAAAAAAAAABkARQnAQ',
Copy link
Member

Choose a reason for hiding this comment

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

这里赞

@hustcc
Copy link
Member

hustcc commented Sep 24, 2020

合并了，下一个分支，可以把单测覆盖率提升一下，然后学习一下写有效的单元测试。一般：

  • util 等这类纯函数一定是要 100% 覆盖的，如果做不到 100% 覆盖，说明有一些 dead code 可以移除
  • 其他非纯函数的代码，因为构造环境比较麻烦，但是也需要保证在 85% + 的覆盖率的。

@hustcc hustcc merged commit 5f99d95 into antvis:master Sep 24, 2020
@zhangzhonghe
Copy link
Member Author

  • dead code

明白了👍

@zhangzhonghe zhangzhonghe deleted the imageMask/word-cloud branch September 26, 2020 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lxfu1 lxfu1 lxfu1 left review comments

@hustcc hustcc hustcc approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request PR: merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhangzhonghe @hustcc @lxfu1