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

fix(badge): fix badge number is rtl #43998

Merged
merged 7 commits into from Aug 14, 2023
Merged

fix(badge): fix badge number is rtl #43998

merged 7 commits into from Aug 14, 2023

Conversation

NotEvenANeko
Copy link
Contributor

@NotEvenANeko NotEvenANeko commented Aug 3, 2023 •
edited

[中文版模板 / Chinese template]

🤔 This is a ...

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

🔗 Related issue link

close #43971

💡 Background and solution

Number in badge is right-to-left if direction of the page is RTL, e.g. 25 displays as 52, 999+ displays as +999.

Wrap the number with a <bdi> to fix it.

📝 Changelog

Language Changelog
🇺🇸 English Fix number in badge is RTL when page is RTL
🇨🇳 Chinese 修复了当页面的文字方向为 RTL 时 Badge 里面的数字也是 RTL 的问题

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • 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

🚀 Summary

🤖 Generated by Copilot at 826024d

Wrap badge numbers in <bdi> element to preserve their directionality in different languages. Improve internationalization of components/badge/ScrollNumber.tsx.

🔍 Walkthrough

🤖 Generated by Copilot at 826024d

  • Wrap the number nodes in a <bdi> element to prevent directionality issues in some languages (link)

@stackblitz
Copy link

stackblitz bot commented Aug 3, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023 •
edited

@argos-ci
Copy link

argos-ci bot commented Aug 3, 2023 •
edited

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No change detected - Aug 14, 2023, 9:57 AM

@li-jia-nan
Copy link
Member

@afc163
Copy link
Member

afc163 commented Aug 3, 2023

https://github.com/ant-design/ant-design/actions/runs/5748733282/job/15582496674?pr=43998

@NotEvenANeko
Copy link
Contributor Author

I forgot to update the snapshots, fixed.

@li-jia-nan
Copy link
Member

第一次见这个标签，长姿势了

@li-jia-nan
Copy link
Member

话说这个不会影响到正常模式吧？

@NotEvenANeko
Copy link
Contributor Author

不会吧，就只是和 dir="auto" 一样，把里面的 direction 和外面的隔离开了

@li-jia-nan
Copy link
Member

@afc163 这个 dom 结构变了，算不算 break change？是不是得发到 feature 分支？

@MadCcc
Copy link
Member

MadCcc commented Aug 3, 2023

加个 direction: rtl 是不是就可以了。动 dom 结构容易 break

@NotEvenANeko
Copy link
Contributor Author

css 的 direction 只能是 ltr 或者 rtl，如果里面有 rtl 的内容那么设置为 ltr 感觉也不太好

不过也许不会有 rtl 的内容？

之前试过 dir="auto" 但是会影响排版，可能可以再试一下这个

@MadCcc
Copy link
Member

MadCcc commented Aug 3, 2023

加一些用例看看么，bdi 如果合适的话可以把 span 替换掉

@codecov
Copy link

codecov bot commented Aug 3, 2023 •
edited

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (ee984fe) 100.00% compared to head (0ea673d) 100.00%.

Additional details and impacted files 
@@            Coverage Diff            @@
##            master    #43998   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          659       659           
  Lines        11200     11200           
  Branches      3033      3033           
=========================================
  Hits         11200     11200
Files Changed Coverage Δ
components/badge/style/index.ts 100.00% <ø> (ø)
components/badge/ScrollNumber.tsx 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NotEvenANeko
Copy link
Contributor Author

是要什么样的用例呢

bdi 和 span 替换掉的话应该不会有什么问题，这两个元素的分类和 content model 都是一样的，不过需要替换掉的是默认的 sup，语义就变了

@MadCcc
Copy link
Member

MadCcc commented Aug 3, 2023

如果里面有 rtl 的内容那么设置为 ltr 感觉也不太好

主要是这个，bdi 应该是对整个 children 生效的，所以实际上和加 direction: ltr 应该是等价的？

@NotEvenANeko
Copy link
Contributor Author

bdi 只是隔离文本方向让浏览器对 bdi 里面的元素重新计算，里面也可能会有 rtl 的内容，那样感觉 direction: ltr 就不太合适了

@NotEvenANeko
Copy link
Contributor Author

用 dir="auto" 做了一下，如果 sup 内外文字方向一样的话不会有问题，如果不一样的话设置的 inset-inline-end: 0 会以 sup 元素的方向为基础，如果是 LTR 的内容的话就是 LTR 下面的 right: 0，然后和 &-rtl 里面的 transform: translate(-50%, -50%) 就让这个 badge 向左错位了

然后就在 &-rtl 里面设置 inset-inline-end: auto; inset-inline-start: 0 来把 badge 移到 LTR 意义上的左上角，但是在内容为 RTL 的时候这样子就不行了会错位了，如果可以拿到 sup 的 direction 的计算值的话就可以做成 LTR RTL 都兼容的了，就是定位 badge 是同时需要元素内和元素外的文字方向

把 badge 最外层的 span 换成 bdi 也是一样的问题

加一层 bdi 隔离文字方向也许是最简单的方法，就是会改动 dom 结构

感觉这么说有点不是很直观……我把给改完但是有点问题的 push 上来？

@MadCcc
Copy link
Member

MadCcc commented Aug 4, 2023

我明白了。
还有一种解法是给数字节点的 sup 添加额外的 className，这样就可以设置 direction: ltr 且不会影响 dom。
这样做是否会好些？

@NotEvenANeko
Copy link
Contributor Author

这样就和给 sup 加 dir="auto" 是一样的了，都需要调整样式让 badge 在 RTL 的时候会在左上角，不然就会这样子

image

zombieJ
zombieJ reviewed Aug 4, 2023
View reviewed changes
components/badge/ScrollNumber.tsx Outdated
@@ -79,7 +79,7 @@ const ScrollNumber = React.forwardRef<HTMLElement, ScrollNumberProps>((props, re

return (
<Component {...newProps} ref={ref}>
{numberNodes}
<bdi>{numberNodes}</bdi>
Copy link
Member

Choose a reason for hiding this comment

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

bdi IE 是不支持的，可以做兼容的就尽量做兼容吧：

截屏2023-08-04 11 10 11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

要兼容 IE 的话 dir="auto" 也是不能使用的就只能 direction: ltr，这样就需要假设 badge 里面的内容只会是 LTR 的了

@MadCcc
Copy link
Member

MadCcc commented Aug 7, 2023 •
edited

研究了一下现在会产生问题是因为 ScrollNumber 把数字拆开来了
image
而如果是一个元素的话就不会有问题
image

所以实际上只要把 ScrollNumber 的方向强制设置成 ltr 就可以了

@Yuiai01
Copy link
Contributor

Yuiai01 commented Aug 8, 2023

研究了一下现在会产生问题是因为 ScrollNumber 把数字拆开来了 image 而如果是一个元素的话就不会有问题 image

所以实际上只要把 ScrollNumber 的方向强制设置成 ltr 就可以了

#43991 感觉也可以用这个解法，给 rtl 模式下内部的数字设置一个 float: right 。

@NotEvenANeko
Copy link
Contributor Author

感觉 bdi 语义上明确一点，稍微差一点就是 dir="auto"，css 就没有什么语义了感觉

@zombieJ
Copy link
Member

zombieJ commented Aug 10, 2023

感觉 bdi 语义上明确一点，稍微差一点就是 dir="auto"，css 就没有什么语义了感觉

dir="auto" 吧，搞完合了~

@NotEvenANeko
Copy link
Contributor Author

我晚上搞一下

@MadCcc
Copy link
Member

MadCcc commented Aug 10, 2023

直接改 ScrollNumber 的 style 就可以了吧

This was referenced Aug 31, 2023
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Sep 7, 2023
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MadCcc MadCcc MadCcc left review comments

@zombieJ zombieJ zombieJ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RTL Badge revert value number
6 participants
@NotEvenANeko @li-jia-nan @afc163 @MadCcc @Yuiai01 @zombieJ