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

className prop ordering #26602

Merged
merged 2 commits into from Sep 6, 2020
Merged

className prop ordering #26602

merged 2 commits into from Sep 6, 2020

Conversation

x1mrdonut1x
Copy link
Contributor

@x1mrdonut1x x1mrdonut1x commented Sep 5, 2020

🤔 This is a ...

  • Component style update

🔗 Related issue link

#26594

💡 Background and solution

Currently most of the Ant Design components are defined as follows:

const class = classNames(userDefinedClassName, componentClassName, ...)

Which makes ant classNames take precedence over user defined classNames. As a user, I would like to easily override ant-d classNames.

☑️ 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

resolve #26594

@x1mrdonut1x x1mrdonut1x changed the title className prop ordering #26594 className prop ordering Sep 5, 2020
@ant-design-bot
Copy link
Contributor

ant-design-bot commented Sep 5, 2020

@ant-design-bot
Copy link
Contributor

ant-design-bot commented Sep 5, 2020

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 5, 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 0c3dc16:

Sandbox Source
antd reproduction template Configuration

@codecov
Copy link

codecov bot commented Sep 5, 2020

Codecov Report

Merging #26602 into master will increase coverage by 1.43%.
The diff coverage is 99.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26602      +/-   ##
==========================================
+ Coverage   98.38%   99.82%   +1.43%     
==========================================
  Files         364      383      +19     
  Lines        7320     7348      +28     
  Branches     1957     2055      +98     
==========================================
+ Hits         7202     7335     +133     
+ Misses        118       13     -105     
Impacted Files Coverage Δ
components/_util/easings.ts 100.00% <ø> (ø)
components/_util/getRenderPropValue.ts 100.00% <ø> (ø)
components/_util/hooks/usePatchElement.tsx 100.00% <ø> (ø)
components/_util/openAnimation.tsx 96.42% <ø> (ø)
components/_util/ref.ts 100.00% <ø> (+11.11%) ⬆️
components/button/LoadingIcon.tsx 100.00% <ø> (ø)
components/cascader/index.tsx 100.00% <ø> (+2.33%) ⬆️
components/checkbox/Checkbox.tsx 100.00% <ø> (ø)
components/checkbox/Group.tsx 100.00% <ø> (ø)
components/collapse/Collapse.tsx 100.00% <ø> (ø)
... and 204 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 54d0503...0c3dc16. Read the comment docs.

@afc163 afc163 merged commit bf45c4c into ant-design:master Sep 6, 2020
@pr-triage pr-triage bot added the PR: merged label Sep 6, 2020
@lich-yoo
Copy link
Contributor

lich-yoo commented Sep 9, 2020

@afc163 @zombieJ 不懂更改顺序会有什么变化,请指教下。

比如其中的一个 change

-    const wrapperClass = classNames(className, `${prefixCls}-wrapper`, {
-      [`${prefixCls}-rtl`]: direction === 'rtl',
-   });

+    const wrapperClass = classNames(
+      `${prefixCls}-wrapper`,
+      {
+        [`${prefixCls}-rtl`]: direction === 'rtl',
+      },
+      className,
+    );

className 只能增量,不能删减antd的样式吧?覆盖也是没意义吧?就是一个 string 而已,相当于没变化。
而且改了 80 多个文件,感觉手抖一个地方就完蛋。

@lich-yoo
Copy link
Contributor

lich-yoo commented Sep 9, 2020

而且在同一个节点的 classname 顺序不同,应该优先级不变;
https://codesandbox.io/s/nifty-dew-vopvz?file=/src/styles.css

image

@yoyo837
Copy link
Contributor

yoyo837 commented Sep 9, 2020

好像还是有点区别的,当两个css的选择器优先级不同时

@lich-yoo
Copy link
Contributor

lich-yoo commented Sep 9, 2020

好像还是有点区别的,当两个css的选择器优先级不同时

其实这个 merge 改的就是 classNames 的参数顺序,最终也就是 className 字符串中的顺序,
https://stackoverflow.com/questions/1321692/how-to-specify-the-order-of-css-classes 这个来看,

class 属性的顺序没关系 ×
在 css 中定义的前后位置有关系 √

@afc163
Copy link
Member

afc163 commented Sep 9, 2020

class 属性的顺序没关系 ×

你是对的,确实没关系。

改就改了吧。

@lich-yoo
Copy link
Contributor

lich-yoo commented Sep 9, 2020

ok。

@yoyo837
Copy link
Contributor

yoyo837 commented Sep 9, 2020

也还是应该放后面
:语义 、css module等看上去更合适

ant ant-model ant-model__trans

ant-model__trans ant-model ant

@afc163
Copy link
Member

afc163 commented Sep 9, 2020

对的,视觉上从前往后第一眼能看出来是什么,避免看到一堆自定义的 className。

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.

className prop ordering
5 participants