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

Textコンポーネントの改修 #312

Closed
wants to merge 6 commits into from
Closed

Textコンポーネントの改修 #312

wants to merge 6 commits into from

Conversation

takashi0602
Copy link
Member

@takashi0602 takashi0602 commented Oct 15, 2023

ref: #305

概要

Figmaのデザイン刷新によるTextコンポーネントの改修を行いました。確認お願いします🙏

Storybook上でTextコンポーネントを描画している画像

コード解説

今まではvanilla-extractのRecipes機能を用いて、予め定義していたサイズ(xsなど)を使用するようにしていましたが、デザインを刷新したことによって定義していたサイズ以外の組み合わせが出てきました。そのため、Dynamic機能を用いて動的にフォントサイズを変更できるようにしました。
Dynamic機能自体はstyle属性でCSS変数を定義し、その定義したCSS変数を用いてスタイリングするような形となります。

また、今までレスポンシブ対応はSprinkles機能を通じて行っていましたが、SprinklesとDynamicは一緒に扱うことができず、やむを得ずsrc/styles/breakpoint.ts内にレスポンシブ対応を行うための関数を用意し、それを用いることでDynamic機能でもレスポンシブ対応を実現しております。

@takashi0602 takashi0602 self-assigned this Oct 15, 2023
@uyupunpopunpo
Copy link
Contributor

@tyokinuhata
Copy link
Member

今まではvanilla-extractのRecipes機能を用いて、予め定義していたサイズ(xsなど)を使用するようにしていましたが、デザインを刷新したことによって定義していたサイズ以外の組み合わせが出てきました。そのため、Dynamic機能を用いて動的にフォントサイズを変更できるようにしました。

これってサイズの組み合わせが増えたからDynamicsを使わないとかえって複雑になりそうだったみたいな話?

src/styles/breakpoint.ts Outdated Show resolved Hide resolved
src/styles/themes.css.ts Show resolved Hide resolved
src/styles/themes.css.ts Show resolved Hide resolved
Comment on lines +71 to +74
fontSize: {
mobile: vars.fontSize[fontSize.mobile],
desktop: vars.fontSize[fontSize.desktop],
},
Copy link
Member

Choose a reason for hiding this comment

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

よくわかってないんだけど、フォントサイズを動的に指定できるようにはなったけど、varsに定義されていないフォントサイズが指定されたら駄目ってこと?

Copy link
Member Author

Choose a reason for hiding this comment

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

ですね。
動的に指定はできるけど、定義されたフォントサイズ内で指定してね🫶というルールみたいなもんっすね。
俺以外の人がコード書くことがあった時に便利かなと

Copy link
Member

Choose a reason for hiding this comment

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

これ定義されてないフォントサイズを指定したらどういう挙動をするんですか?
あとコンポーネントを使う側から見ると、そのルールが暗黙的に見えるのがイマイチなのかなーと思うんですよねえ

Copy link
Member Author

Choose a reason for hiding this comment

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

ts使ってる上、定義されてないフォントサイズ指定したらエラーになるので、実行前に検知できる感じっすね
暗黙的に見えるかもしれないけど、使うときには↑の挙動になるんで、問題ないのかな〜という感じはしてる

Copy link
Member

Choose a reason for hiding this comment

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

まあ良いのかなあ。裏側で変数定義してるならわざわざ自由な値を渡せる設計にするメリットもあまり無いような気はしたけど

Copy link
Member

Choose a reason for hiding this comment

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

ワロタ、まあ今後増える可能性はあるけどいうてそこまでは増えないか。

Copy link
Member

Choose a reason for hiding this comment

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

src/styles/themes.css.tsfontSize には10種類ぐらいあると思うけど、この全種類に対応させる必要はないのか。

Copy link
Member Author

Choose a reason for hiding this comment

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

全種類に対応する必要はないっすね
タイトルは別でコンポーネントあったりとかやし

Copy link
Member Author

@takashi0602 takashi0602 Oct 21, 2023

Choose a reason for hiding this comment

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

このPRはCloseして別PRで対応します🙏
(不要なコミット含めちゃうので)

Copy link
Member

Choose a reason for hiding this comment

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

なるほど、クローズおねがいします

@takashi0602
Copy link
Member Author

これってサイズの組み合わせが増えたからDynamicsを使わないとかえって複雑になりそうだったみたいな話?

ですね。
サイズの組み合わせが増えたのと、サイズが増えすぎると返ってコンポーネント使う時にややこしくなりそうなので、Dynamics使ってコンポーネント定義する際にフォントサイズを指定するように変えました🙏

@takashi0602
Copy link
Member Author

PR #316 で別途対応したため、このPRはCloseします。

@tyokinuhata tyokinuhata deleted the feat/305_text branch October 24, 2023 02:32
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