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(Row): use getCssVarsValue function, and comparing sizes with units #2734

Closed
wants to merge 2 commits into from

Conversation

topazur
Copy link

@topazur topazur commented Jan 28, 2024

🤔 这个 PR 的性质是?

  • 日常 bug 修复
  • 新特性提交
  • 文档改进
  • 演示代码改进
  • 组件样式/交互改进
  • CI/CD 改进
  • 重构
  • 代码风格优化
  • 测试用例
  • 分支合并
  • 其他

🔗 相关 Issue

#2733

💡 需求背景和解决方案

📝 更新日志

  • fix(组件名称): 处理问题或特性描述 ...

  • 本条 PR 不需要纳入 Changelog

☑️ 请求合并前的自查清单

⚠️ 请自检并全部勾选全部选项⚠️

  • 文档已补充或无须补充
  • 代码演示已提供或无须提供
  • TypeScript 定义已补充或无须补充
  • Changelog 已提供或无须提供

Copy link
Contributor

完成

@HaixingOoO
Copy link
Collaborator

您好,正式版不要重大修改,会影响到以前的,兼容处理好点

src/_util/dom.ts Outdated
if (!canUseDocument) return;
export function getCssVarsValue(name: string, withUnit?: false, element?: HTMLElement): number | undefined;
export function getCssVarsValue(name: string, withUnit: true, element?: HTMLElement): string | undefined;
export function getCssVarsValue(name: string, withUnit: any, element?: HTMLElement): any {
Copy link
Collaborator

Choose a reason for hiding this comment

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

对于内部来说可能算是一个 bug 在内部,但是这个函数暴露到外面后,用户可能使用了这个函数进行数据处理,现在新增的变动可能导致一部分用户的使用方法被破坏。
如果你是想在 TDesign 内部使用的话,可以考虑重命名一个新的方法,然后代理新的方法。如果你是想在自己的项目使用的话,需要处理一下函数的兼容问题防止 break 到其他的用户,正式版本不做破坏性变更。

Copy link
Author

Choose a reason for hiding this comment

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

已修改

Comment on lines -22 to -26
const smWidth = getCssVarsValue('--td-screen-sm') || 768;
const mdWidth = getCssVarsValue('--td-screen-md') || 992;
const lgWidth = getCssVarsValue('--td-screen-lg') || 1200;
const xlWidth = getCssVarsValue('--td-screen-xl') || 1400;
const xxlWidth = getCssVarsValue('--td-screen-xxl') || 1880;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@NWYLZW 一介哥,这里是不是直接parseFloat(getCssVarsValue('--td-screen-sm'))就好了,不用新增函数加那么多判断?

export const getCssVarsValue = (name: string, element?: HTMLElement) => {
if (!canUseDocument) return;
export const getCssVarsValue = (name: string, element?: HTMLElement): string | undefined => {
if (!canUseDocument) return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!canUseDocument) return undefined;
if (!canUseDocument) return;

没有实际意义的修改

@@ -202,13 +202,22 @@ export const getAttach = (node: any): HTMLElement => {
};

// 获取 css vars
export const getCssVarsValue = (name: string, element?: HTMLElement) => {
if (!canUseDocument) return;
export const getCssVarsValue = (name: string, element?: HTMLElement): string | undefined => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const getCssVarsValue = (name: string, element?: HTMLElement): string | undefined => {
export const getCssVarsValue = (name: string, element?: HTMLElement) => {

一定需要主动声明类型吗,有什么具体的需求吗,ts 在这里能主动推断的吧?

Copy link
Author

Choose a reason for hiding this comment

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

这里推断的是string,都没有undefined的情况

Copy link
Collaborator

Choose a reason for hiding this comment

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

但是在 206 出现了 undefined,return 在没有指定变量的时候返回的是 undefined。

https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABMOcAUEEBMBciBGqANgKYCGYAlIgN4BQijiA9M0wHoD8DTMwiGbNQBOJKCGFgA3D0ajxkxAEYZAXyA
image

Copy link
Author

Choose a reason for hiding this comment

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

image 我不知道是不是ts版本的问题

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Author

Choose a reason for hiding this comment

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

image 我这边不管是函数声明还是使用,都没有推断出来。先解决 bug 吧

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个是另外的问题,并不是 undefined 导致的,可以直接按照 parseFloat 的修改方法处理一下就好了。待会儿我看看

Copy link
Author

Choose a reason for hiding this comment

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

行!所以我尝试主动声明类型了

);
const xxlWidth = getCssVarsValueFormat<number>('--td-screen-xxl', undefined, (val?: string) =>
val ? Number.parseFloat(val) : 1880,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

按照上面说的 parseFloat(getCssVarsValue('--td-screen-sm') || 768) 改一下就好了吧?没必要写的这么复杂?

Copy link
Author

Choose a reason for hiding this comment

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

我只想快点修复,我需要用到这个变量,不想外部再监听一遍。我是按照 代理新的方法 这里修改的。我关闭pr了,希望能尽快修复,很简单的一个问题

@topazur topazur closed this Jan 30, 2024
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