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

optimize (components): ButtonIcon with smarter class merging and tippy #463

Merged
merged 4 commits into from
Jun 1, 2024

Conversation

orangelckc
Copy link
Contributor

描述

ButtonIcon 图标按钮需要同时支持默认样式和自定义样式,当存在自定义样式时,应该覆盖那些和默认样式相同的类,保留默认样式中与自定义样式中不重叠的类,原代码中使用计算属性 cls 解决这个问题

https://github.com/soybeanjs/soybean-admin/blob/086bad474eebfa38c91c7795d280251cc15de1fd/src/components/custom/button-icon.vue#L37C1-L49C4

const cls = computed(() => {
  let clsStr = props.class;


  if (!clsStr.includes('h-')) {
    clsStr += ' h-36px';
  }


  if (!clsStr.includes('text-')) {
    clsStr += ' text-icon';
  }


  return clsStr;
});

例:默认 h-36px,如果传入 h-40px 那么应该覆盖默认类,而默认的 text-icon 没有和自定义样式重叠的部分,需要保留,那么最后的DOM应该为 class="h-40px text-icon"

但是,这样的写法并没有很好的扩展性,当组件每多一个默认样式类,就需要多一个 if 判断,而且对于 bg-red-300 这种形式的类名使用 includes 方法来进行判断就不是很适合了。

解决

使用 tailwind-merge 库来处理 css 合并时需要处理的逻辑,他的功能描述与我们的需求 100% 契合

image

新的问题

项目中使用了 h-36px 而不是 h-[36px]h-9 的方式声明类名,而后面的2种方式才可以被 tailwind-merge 识别并自动合并类,故代码中写为 h-[36px],这是由于 unocsstailwindcss 语法差异导致的,经测试能达到需要的效果。

关于使用 tailwind-mergeunocss 框架下的可行性讨论,目前认为是 可以使用 的,

详见,unocss/unocss#2748

其他修改

  • 对于按钮是否显示 tooltip 的逻辑可以使用 disabled 属性来直接控制,不在需要做 v-if 的判断,对相关代码进行了简化

PS:因为对项目还不是很熟悉,可能对相关 ButtonIcon 的引用测试不到位,还请 CR 的时候帮忙测试一下~

package.json Outdated
@@ -88,6 +88,7 @@
"lint-staged": "15.2.2",
"sass": "1.77.2",
"simple-git-hooks": "2.11.1",
"tailwind-merge": "^2.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

这个依赖安装到 dependencies 中

@honghuangdc
Copy link
Member

我觉得ok
有个依赖的问题,解决一下就行

@honghuangdc honghuangdc merged commit 3ad4389 into soybeanjs:main Jun 1, 2024
1 check passed
@orangelckc orangelckc deleted the kc/merge-class branch June 1, 2024 08:56
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.

2 participants