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

トグルボタンのデザインを変更 #260

Merged
merged 9 commits into from
Mar 25, 2021
Merged

トグルボタンのデザインを変更 #260

merged 9 commits into from
Mar 25, 2021

Conversation

hirokikondo86
Copy link
Contributor

@hirokikondo86 hirokikondo86 commented Mar 18, 2021 •
edited

Ref

#121

やったこと

  • ノブに立体感を出した
  • 本体を少し凹ませた表現にするためインナーシャドウを加えた
  • トグルボタンがテキストの下側に埋もれていたため、z-indexを変更した

スクショ

Before

image

After

image

@hirokikondo86 hirokikondo86 requested a review from a team as a code owner March 18, 2021 01:53
@hirokikondo86 hirokikondo86 requested review from penicillin0 and removed request for a team and penicillin0 March 18, 2021 01:53
@hirokikondo86 hirokikondo86 self-assigned this Mar 18, 2021
@noronaoki
Copy link
Contributor

@hirokikondo86
長めのテキストを入れると左右の空白にかなり余裕を持たせないとテキストが折り返しちゃうのがもったいない。
これは元々の実装がこうなっているのだけども、せっかくのタイミングなのでもっと適切な実装方法が無いか検討するのが良さそう。
image

@hirokikondo86
Copy link
Contributor Author

hirokikondo86 commented Mar 18, 2021 •
edited

これは例えば、子要素（テキスト）の幅に合わせて親要素（ボタン自体）の幅も可変させる様な実装するといいかもねって話ですよね…？👀

@noronaoki
Copy link
Contributor

それができたら理想だね。
でも現状の作りだと横幅も事前に指定が必要な設計だもんねぇ。

@hirokikondo86
Copy link
Contributor Author

そうなんですよねー…。
恐らくそれを実現しようと思うと、ゴリっと設計を変える必要があるのかな？って思いました。

@hirokikondo86
Copy link
Contributor Author

hirokikondo86 commented Mar 18, 2021 •
edited

@noronaoki
テキスト幅に合わせたボタンサイズに可変する実装に成功したので、反映しておきました！
また、デフォルトで渡していたwidthを削除した実装に変更したので、ご確認よろしくお願いしますmm

@hirokikondo86
Copy link
Contributor Author

hirokikondo86 commented Mar 18, 2021 •
edited

あ、現状のテキストだと確認ができないため、長めのテキストに変更しておきました🙇‍♂️

@noronaoki
Copy link
Contributor

@hirokikondo86 これだと例えばONのほうを長めのテキストにしたとき、OFFのほうが短いままだとスイッチしたときボタンの長さがガチャガチャ変わっちゃうよね。

2021-03-18.17.33.49.mov

なのでこの方式を採るならテキストのelementの横幅を取得して大きい方のwidthに揃えるってのをやらなきゃいけないんだけど、問題は切り替えた後にレンダリングされるから横幅を予め取得する術が無さそうというところかな。

@hirokikondo86
Copy link
Contributor Author

hirokikondo86 commented Mar 18, 2021 •
edited

memo:

  • toggleに関してはライブラリなどは使っておらず、ゼロから実装していた
  • ON/OFFテキストがノブよりも上に位置していたのと切り替えの都度re-paintが走る仕様になっていたのでアニメーションに違和感があった
  • ON/OFFのエレメントを2つ用意する方式にしてみる
  • アニメーションをもっとスムースなものにできないか検証してみる（野呂）
  • その間方式を変えるやつで実装を検証してみる（gorinya）
  • widthの設定など、破壊的な変更が起きないようにする方向でいく

@hirokikondo86
Copy link
Contributor Author

  • テキストの表示/非表示アニメーションをopacityでスムースにした
  • トグルボタンをテキストの上に持ってきた
  • テキストが長くなったときの左右の大きめのpaddingを削除

@noronaoki
Copy link
Contributor

@hirokikondo86
全体的に詰めが甘いのでサンプルをもうちょいちゃんと見て。

  • transition durationが短いので動作が速い
  • テキストにtransitionが設定されていない
  • ONの時のノブのborderカラーが違う

@hirokikondo86
Copy link
Contributor Author

指摘箇所に追加でテキストのcolorにもdurationを適用しました！🙇‍♂️

@hirokikondo86
Copy link
Contributor Author

また、ボタンの横幅を超えたときのテキストに

word-break: break-all;
white-space: nowrap;

が効いていなかったので、追加で修正しときました！🙇‍♂️

@noronaoki
Copy link
Contributor

@hirokikondo86 LGTM!

ryokosuge
ryokosuge reviewed Mar 22, 2021
View reviewed changes
src/components/ToggleButton/styled.ts Outdated
Comment on lines 41 to 55
export const LabelText = styled.div`
position: absolute;
width: 100%;
word-break: break-all;
white-space: nowrap;
transition: all 0s 0.3s cubic-bezier(0.47, 0, 0.75, 0.72);
`;
export const ActiveLabelText = styled(LabelText)`
padding-right: 6px;
opacity: 0;
`;
export const InActiveLabelText = styled(LabelText)`
margin-left: 6px;
opacity: 1;
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

`;
// ここに改行入れて欲しい
export const

めちゃくちゃ細かいんだけど、みっちり詰めすぎて可読性下がってるので改行挟んで欲しいなー
このファイルの他のところは export const の上は1行改行含まれてるので...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryokosuge
確かに…！修正しました！

@ryokosuge ryokosuge self-requested a review March 23, 2021 06:34
@hirokikondo86 hirokikondo86 merged commit 4c0c4d6 into master Mar 25, 2021
@hirokikondo86 hirokikondo86 deleted the update/toggle-btn-layout branch March 25, 2021 03:19
@hirokikondo86 hirokikondo86 restored the update/toggle-btn-layout branch March 25, 2021 06:20
@hirokikondo86 hirokikondo86 mentioned this pull request Mar 25, 2021
Merged
@hirokikondo86 hirokikondo86 deleted the update/toggle-btn-layout branch March 30, 2021 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryokosuge ryokosuge ryokosuge approved these changes

@noronaoki noronaoki Awaiting requested review from noronaoki

@maktak1995 maktak1995 Awaiting requested review from maktak1995

Assignees

@hirokikondo86 hirokikondo86

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hirokikondo86 @noronaoki @ryokosuge