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: alert close padding #24471

Merged
merged 3 commits into from May 27, 2020
Merged

fix: alert close padding #24471

merged 3 commits into from May 27, 2020

Conversation

xrkffgg
Copy link
Member

@xrkffgg xrkffgg commented May 26, 2020

[中文版模板 / Chinese template]

🤔 This is a ...

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

🔗 Related issue link

💡 Background and solution

image

@padding-xs 是 8px

📝 Changelog

Language Changelog
🇺🇸 English Fix Alert close icon padding.
🇨🇳 Chinese 修复 Alert 关闭按钮 padding

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

@xrkffgg
Copy link
Member Author

@xrkffgg xrkffgg commented May 26, 2020

@AshoneA 下次改的时候,记得改 RTL 鸭

@ant-design-bot
Copy link
Contributor

@ant-design-bot ant-design-bot commented May 26, 2020

@ant-design-bot
Copy link
Contributor

@ant-design-bot ant-design-bot commented May 26, 2020

@@ -856,6 +856,8 @@
@alert-with-description-padding-vertical: @padding-md - 1px;
@alert-with-description-padding: @alert-with-description-padding-vertical 15px
@alert-with-description-no-icon-padding-vertical - 1px 64px;
@alert-with-description-padding-rtl: @alert-with-description-padding-vertical 64px
Copy link
Member

@afc163 afc163 May 26, 2020

Choose a reason for hiding this comment

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

不需要这个变量,应该只把 64px 单独提成变量。

Copy link
Member Author

@xrkffgg xrkffgg May 26, 2020

Choose a reason for hiding this comment

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

😂😂 叫啥名字好呢

Copy link
Member

@afc163 afc163 May 26, 2020

Choose a reason for hiding this comment

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

上面这个 @alert-with-description-padding 也不好,但是不好改了。

Copy link
Member

@afc163 afc163 May 26, 2020

Choose a reason for hiding this comment

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

64px 改成从这三个算出来吧,两个 padding 加一个 icon size。

image

Copy link
Member Author

@xrkffgg xrkffgg May 26, 2020

Choose a reason for hiding this comment

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

那要去掉 @alert-with-description-padding 这个变量 吗?直接放回 alert/index 里

Copy link
Member

@afc163 afc163 May 26, 2020

Choose a reason for hiding this comment

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

也别删了,免得 breaking change,直接在这里算。

@codecov
Copy link

@codecov codecov bot commented May 26, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24471   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files         363      363           
  Lines        7261     7261           
  Branches     2009     2009           
=======================================
  Hits         7192     7192           
  Misses         69       69           

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 1bf9959...46475be. Read the comment docs.

@afc163
Copy link
Member

@afc163 afc163 commented May 26, 2020

image

和 RTL 有关系么

@xrkffgg
Copy link
Member Author

@xrkffgg xrkffgg commented May 26, 2020

和 RTL 有关系么

RTL 只是换成了变量。其实没太大关系。

@xrkffgg
Copy link
Member Author

@xrkffgg xrkffgg commented May 26, 2020

不用管 分支名 😂😂😂

@AshoneA
Copy link
Contributor

@AshoneA AshoneA commented May 27, 2020

@AshoneA 下次改的时候,记得改 RTL 鸭

gotcha!

@AshoneA AshoneA merged commit 9e96b9e into master May 27, 2020
36 checks passed
@AshoneA AshoneA deleted the fix-alert-rtl branch May 27, 2020
@zombieJ zombieJ mentioned this pull request May 31, 2020
13 tasks
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.

None yet

5 participants