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(module:icon): switch to SVG icons #2171

Merged
merged 3 commits into from
Oct 8, 2018

Conversation

wzhudev
Copy link
Member

@wzhudev wzhudev commented Sep 20, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Icons based on font files.

Issue Number: N/A

What is the new behavior?

Icons based on SVG.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

But we should make a break change for this in the future.

Other information

localhost_49152_components_icon_zh
localhost_49152_components_icon_zh 1

@wzhudev wzhudev changed the title [WIP] feat() [WIP] feat(module:icon): switch to SVG icons Sep 20, 2018
@Component({
selector: 'nz-demo-icon-basic',
template: `
<i icon [type]="'home'"></i>
Copy link
Member

Choose a reason for hiding this comment

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

个人建议 icon 指令名有一定的意义,例如:

<i icon="home"></i>

我想会更好一点。

Copy link
Member Author

@wzhudev wzhudev Sep 20, 2018

Choose a reason for hiding this comment

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

嗯, 现在 API 的设计只考虑了和 antd 兼容 相似. 这个 API 的确更好一些.

@wzhudev
Copy link
Member Author

wzhudev commented Sep 21, 2018

CSS update in antd 3.8.2 is also included in this pull request for previewing the result.

Cannot provide an online preview because a new dependency hasn't been published to npm yet.

TODO:

@wzhudev
Copy link
Member Author

wzhudev commented Sep 28, 2018

Waiting for @ant-design/icons-angular to release.

Released! 🎉

@wzhudev wzhudev changed the title [WIP] feat(module:icon): switch to SVG icons feat(module:icon): switch to SVG icons Sep 28, 2018
@wzhudev wzhudev force-pushed the refactor-icon branch 4 times, most recently from 17caef5 to c257bbe Compare September 29, 2018 12:38
@codecov
Copy link

codecov bot commented Sep 29, 2018

Codecov Report

Merging #2171 into master will decrease coverage by 0.03%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2171      +/-   ##
==========================================
- Coverage   95.96%   95.92%   -0.04%     
==========================================
  Files         482      485       +3     
  Lines       11956    12122     +166     
  Branches     1594     1621      +27     
==========================================
+ Hits        11473    11628     +155     
- Misses        134      139       +5     
- Partials      349      355       +6
Impacted Files Coverage Δ
components/menu/demo/theme.ts 100% <ø> (ø) ⬆️
components/button/demo/icon.ts 100% <ø> (ø) ⬆️
components/layout/demo/custom-trigger.ts 100% <ø> (ø) ⬆️
components/layout/demo/side.ts 100% <ø> (ø) ⬆️
components/menu/demo/sider-current.ts 100% <ø> (ø) ⬆️
components/modal/nz-modal.service.ts 91.17% <ø> (ø) ⬆️
components/layout/demo/responsive.ts 100% <ø> (ø) ⬆️
components/button/demo/size.ts 100% <ø> (ø) ⬆️
components/menu/demo/inline.ts 100% <ø> (ø) ⬆️
components/button/demo/loading.ts 100% <ø> (ø) ⬆️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e11247c...092609f. Read the comment docs.

@wzhudev wzhudev mentioned this pull request Sep 29, 2018
3 tasks
@wzhudev wzhudev force-pushed the refactor-icon branch 4 times, most recently from d295a7b to 7e0a86c Compare September 30, 2018 01:31
@wzhudev wzhudev force-pushed the refactor-icon branch 2 times, most recently from d8032a6 to ee6b83a Compare September 30, 2018 02:37
@wzhudev
Copy link
Member Author

wzhudev commented Sep 30, 2018

Netlify is still not OK. Because of Node.js version perhaps. I cannot modify it in package.json. 😢 But the demo runs smoothly in local environment, both dev and prod, with Node.js 10.9.0 and @anguar/cli 6.2.1 which requires Node.js above 9.0.0.

If @ant-design/icons/lib/manifest provided an ES module, we wouldn't have to go through these.

However, since we publish the official website from local env, it seems ok.

I solved this problem by upgrading Node.js in Netlify to 10.9.0. The reason is what I mentioned above.

@wzhudev wzhudev force-pushed the refactor-icon branch 2 times, most recently from 5190e96 to 145e2b3 Compare September 30, 2018 04:49

### 自定义 font 图标

在 `1.7.0` 里,我们提供了一个 `fetchFromIconfont` 方法,方便开发者调用在 iconfont.cn 上自行管理的图标。
Copy link
Member

Choose a reason for hiding this comment

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

从 1.7.0 版本开始


> `?` 为可选。
在 `1.7.0` 版本后,我们与 Ant Design `3.9.x` 同步,使用了 svg 图标替换了原先的 font 图标,从而带来了以下优势:
Copy link
Member

Choose a reason for hiding this comment

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

从 1.7.0 版本开始

components/icon/doc/index.zh-CN.md Show resolved Hide resolved

- 实心和描线图标保持同名,用 `-o` 来区分,比如 `question-circle`(实心) 和 `question-circle-o`(描线);
- 命名顺序:`[图标名]-[形状?]-[描线?]-[方向?]`。
### Svg 图标
Copy link
Member

Choose a reason for hiding this comment

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

SVG

@vthinkxie vthinkxie merged commit 7bdb79b into NG-ZORRO:master Oct 8, 2018
@wzhudev wzhudev deleted the refactor-icon branch October 8, 2018 08:34
hsuanxyz pushed a commit to hsuanxyz/ng-zorro-antd that referenced this pull request Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants