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

feat(design): implement Rate #40

Merged
merged 10 commits into from
Mar 6, 2022

Conversation

Ayase-252
Copy link
Contributor

@Ayase-252 Ayase-252 commented Mar 3, 2022

  • 增加使用 svg 图标的能力;
  • 实现 Rate 组件

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #40 (8cd7100) into master (3a56e8b) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   98.97%   99.05%   +0.08%     
==========================================
  Files          12       13       +1     
  Lines         389      422      +33     
  Branches       43       47       +4     
==========================================
+ Hits          385      418      +33     
  Misses          4        4              
Impacted Files Coverage Δ
packages/design/components/Rate/index.tsx 100.00% <100.00%> (ø)
packages/design/index.tsx 100.00% <100.00%> (ø)

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 3a56e8b...8cd7100. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2022

preview url: https://pr-40--bangumi-next.netlify.app

@Ayase-252
Copy link
Contributor Author

可能需要考虑下怎么把 vite 的配置提升出来,storybook 和 website 可以共享一下配置

@cokemine
Copy link
Contributor

cokemine commented Mar 3, 2022

可能需要考虑下怎么把 vite 的配置提升出来,storybook 和 website 可以共享一下配置

https://www.npmjs.com/package/vite-plugin-package-config 这个应该可以共享一些配置。但是插件之类的应该不行
我觉得可以试试这个能不能用 https://www.npmjs.com/package/webpack-merge

@@ -3,6 +3,8 @@ import { ReactComponent as FilledStar } from './assets/filled-star.svg'
import { ReactComponent as HalfStar } from './assets/half-star.svg'
import { ReactComponent as EmptyStar } from './assets/empty-star.svg'

import styles from './style/index.module.less'
Copy link
Contributor

Choose a reason for hiding this comment

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

是否应该在全局共享组件使用 CSS Modules?
我想的是共享组件使用 普通的类命名方式,使用的时候如果有需要覆盖样式的时候可以直接类似

.in-website-somewhere {
  .bgm-rate__star {
    color: #fd979b;
  }
}

如果组件使用了 CSS Modules, 在用这个组建的时候不知道类名就没办法覆盖样式了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

em,我觉得覆盖组件样式可能不是一个好的实践。当组件库代码掌握在其他人手上的时候,为了快速满足需求是可以用覆盖组件样式的方式实现(其实也不稳定,组件库随便一改class或结构就炸了)。

如果在项目内出现组件不满足需求的情况最好应该是开一个 PR 扩展组件的功能。比如考虑给 Rate 加一个 color 的 props?这样以后 Rate 组件都有换色的能力了。

感觉现在有的几个组件 CSS 这块规范都不太一样。可以讨论讨论,早定下来。

Copy link
Contributor

Choose a reason for hiding this comment

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

感觉组件内用 CSS Module 更简单一点 ,当需要覆盖样式的时候,传入自定义的 className 进行覆盖

Copy link
Contributor

Choose a reason for hiding this comment

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

感觉组件内用 CSS Module 更简单一点 ,当需要覆盖样式的时候,传入自定义的 className 进行覆盖

这个我感觉不太可行。DOM如果是嵌套的,特别是层次比较深的时候针对某一层的元素设置样式不太方便。最多是设置最外层父元素的custom className

Copy link
Contributor

@ubbcou ubbcou Mar 4, 2022

Choose a reason for hiding this comment

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

  • 组件使用CSS module 好像组件库的样式一般不使用module的方式?
  • props 中声明可抽象的属性,如size、color
  • 组件复杂的话还可以传入:wrapperClassName、wrapperStyle之类的props

Copy link
Contributor

@ubbcou ubbcou Mar 4, 2022

Choose a reason for hiding this comment

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

我觉得有必要统计用户的使用环境,能促使决定一些特性是否使用。

回到正题:

  • 需要确定组件CSS规范。例如,CSS module
  • 需要考虑主题色的配置
  • 是否需要确定组件props的定义趋向,例如组件有没有color size type这类props的需求
  • ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

需要考虑主题色的配置
是否需要确定组件props的定义趋向,例如组件有没有color size type这类props的需求

我倒觉得可以在开发过程中慢慢完善这些?

Copy link
Contributor

Choose a reason for hiding this comment

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

首先的要有个起点或者支柱,开发过程中去填充它,这称为完善是没问题的。否则就可能会把这个进程滞后到后期的优化工作。我是这样想的 :D。当然这样做也没问题的

Copy link
Contributor

@ubbcou ubbcou Mar 4, 2022

Choose a reason for hiding this comment

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

我能想到的开发过程中完善项目的方式,可以靠reviewer的检查。

比如上边的例子,可以把color提取到文件顶部声明less变量,后边看看主题色的配置,看看是否可以把这个变量加入到主题里边。

在比如css 规范,可以从review中抽取原子样式(比如整个应用中的圆角、阴影边框——一般来着这种算是统一风格的点吧)?css组件样式?

你们觉得呢。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

em,道理是这个道理。但是在开发前期可能,不太好判断哪些地方是容易变化值得抽象出来的,这样容易进入无限对未来可能性的讨论。这里的相关问题可以在 #42 里讨论了~

@bangumi bangumi locked and limited conversation to collaborators Mar 3, 2022
@bangumi bangumi unlocked this conversation Mar 3, 2022
@trim21
Copy link
Contributor

trim21 commented Mar 3, 2022

抱歉,按错了(

@Ayase-252 Ayase-252 marked this pull request as ready for review March 5, 2022 14:24
@Ayase-252 Ayase-252 merged commit 82607ea into bangumi:master Mar 6, 2022
@Ayase-252 Ayase-252 deleted the feature-design-rating branch March 6, 2022 13:33
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.

5 participants