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

fix: fix ${theme}-theme.js file order #23243

Merged
merged 7 commits into from Apr 15, 2020
Merged

fix: fix ${theme}-theme.js file order #23243

merged 7 commits into from Apr 15, 2020

Conversation

AshoneA
Copy link
Contributor

@AshoneA AshoneA commented Apr 14, 2020

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / document update
  • Component style update
  • TypeScript definition update
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

no

close #23237

💡 Background and solution

貌似上次只是修复了报错,手滑顺序变化了导致以前主题样式配置方式应该是不生效,趁没人提 bug...

📝 Changelog

Language Changelog
🇺🇸 English Fix dark theme and compact theme not working.
🇨🇳 Chinese 修复暗黑主题和紧凑主题不生效的问题。

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

Copy link

@tests-checker tests-checker bot left a comment

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

@ant-design-bot
Copy link
Contributor

@ant-design-bot ant-design-bot commented Apr 14, 2020

@codesandbox-ci
Copy link

@codesandbox-ci codesandbox-ci bot commented Apr 14, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 93beeba:

Sandbox Source
antd reproduction template Configuration

@codecov
Copy link

@codecov codecov bot commented Apr 14, 2020

Codecov Report

Merging #23243 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #23243   +/-   ##
=======================================
  Coverage   98.35%   98.35%           
=======================================
  Files         364      364           
  Lines        7284     7284           
  Branches     1988     1998   +10     
=======================================
  Hits         7164     7164           
  Misses        120      120           

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 2c6733b...93beeba. Read the comment docs.

@zombieJ
Copy link
Member

@zombieJ zombieJ commented Apr 14, 2020

有办法加个测试用例不?Post check 一下。

@afc163
Copy link
Member

@afc163 afc163 commented Apr 14, 2020

需要加个测试用例,不然这块很容易搞错。

@AshoneA
Copy link
Contributor Author

@AshoneA AshoneA commented Apr 14, 2020

@zombieJ @afc163 嗯,我想想

@afc163
Copy link
Member

@afc163 afc163 commented Apr 14, 2020

修复以前主题配置方式不生效

具体是那种配置方式不生效最好写明白。

`const { ${theme}ThemeSingle } = require('./theme');\nconst defaultTheme = require('./default-theme');\n
module.exports = {
...${theme}ThemeSingle,
...defaultTheme
}`,
Copy link

@yunsii yunsii Apr 14, 2020

Choose a reason for hiding this comment

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

这就是写反了的那段代码吧,这样不就导致所有生成的主题都是默认主题了吗? _(:D)∠)_

Copy link
Contributor Author

@AshoneA AshoneA Apr 14, 2020

Choose a reason for hiding this comment

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

嗯,但是不会导致你之前那个的报错。

...darkThemeSingle
}`;
expect(generateThemeFileContent('dark')).toBe(darkContent);
});
Copy link
Member

@afc163 afc163 Apr 14, 2020

Choose a reason for hiding this comment

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

这个测试用例没啥意义。

build 出 dist 以后,require 一下对应 dist 文件,然后加断言,比如暗色主题的某些变量值预期是什么。

Copy link
Contributor Author

@AshoneA AshoneA Apr 14, 2020

Choose a reason for hiding this comment

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

我一开始的想法是用 jest mock 出来 dist 文件,但是卡在怎么把 mock 的 dist 文件变成模块,我想一下怎么做。

Copy link
Contributor Author

@AshoneA AshoneA Apr 14, 2020

Choose a reason for hiding this comment

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

对哦,好像也不用 mock 的样子。

Copy link
Member

@afc163 afc163 Apr 14, 2020

Choose a reason for hiding this comment

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

- run: npm run dist

这后面加个自定义脚本好了。

@@ -19,5 +19,73 @@ describe('antd dist files', () => {
const antd = require('../dist/antd');
expect(antd.version).toBe(pkg.version);
});

/* eslint-disable global-require,import/no-unresolved */

Choose a reason for hiding this comment

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

Please don't disable eslint rules 🙏

Method 1: using Umi 3
#### Method 1

using Umi 3
Copy link
Member

@afc163 afc163 Apr 15, 2020

Choose a reason for hiding this comment

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

Suggested change
using Umi 3
Using Umi 3.

Copy link
Member

@afc163 afc163 Apr 15, 2020

Choose a reason for hiding this comment

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

还是建议标题上带上具体方式,这样右边锚点导航上有信息。

Copy link
Contributor Author

@AshoneA AshoneA Apr 15, 2020

Choose a reason for hiding this comment

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

好像只有二级标题才会出现在右边锚点导航上,这里用二级标题不太合适。

@@ -184,7 +190,7 @@ module.exports = {
};
```

同时开启暗黑和紧凑模式会导致 css 的加载体积增加一倍,这暂时受限于我们目前的主题实现方式,请知晓。
使用 [方式二](#方式二) 同时开启暗黑和紧凑模式会导致 css 的加载体积增加一倍。如果希望体积不变请使用 [方式一](#方式一) 或者 [方式三](#方式三) 引入。
Copy link
Member

@afc163 afc163 Apr 15, 2020

Choose a reason for hiding this comment

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

写在方式二里面就好了。

Copy link
Contributor Author

@AshoneA AshoneA Apr 15, 2020

Choose a reason for hiding this comment

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

突然又犯错了... 方式二根本不支持混合模式,在方式二中更新了说明。

@@ -141,7 +141,9 @@ We have some official themes, try them out and give us some feedback!

![](https://gw.alipayobjects.com/mdn/rms_08e378/afts/img/A*mYU9R4YFxscAAAAAAAAAAABkARQnAQ)

Method 1: using Umi 3
#### Method 1
Copy link
Member

@afc163 afc163 Apr 15, 2020

Choose a reason for hiding this comment

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

保持原来的写法,标题上有内容会比较好找。

docs/react/customize-theme.zh-CN.md Outdated Show resolved Hide resolved
@afc163 afc163 merged commit bff09f8 into master Apr 15, 2020
21 of 27 checks passed
@afc163 afc163 deleted the fix/theme-mode-config branch Apr 15, 2020
xrkffgg pushed a commit that referenced this issue Apr 16, 2020
* fix: fix ${theme}-theme.js file order

* chore: add generateThemeFileContent test

* chore: add test

* patch

* update docs

* update docs

* update docs
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.

5 participants