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

Bug:チェックボックスの位置がズレている #1200

Closed
wants to merge 3 commits into from

Conversation

okakyo
Copy link
Collaborator

@okakyo okakyo commented Jan 27, 2024

タスク

resolve: #1196

行なったこと

  • Checkbox の位置を調整するための props checkboxInputMarginY を追加
  • 使用しなくなった Style Class, 変数を削除

Copy link

changeset-bot bot commented Jan 27, 2024

🦋 Changeset detected

Latest commit: 5c11dd5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@wizleap-inc/wiz-ui-react Minor
@wizleap-inc/wiz-ui-next Minor
@wizleap-inc/wiz-ui-styles Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Jan 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
wiz-ui-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 27, 2024 1:45pm
wiz-ui-react ✅ Ready (Inspect) Visit Preview Jan 27, 2024 1:45pm

Copy link

reg-suit bot commented Jan 27, 2024

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴

🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

Copy link
Contributor

Size Change: +392 B (0%)

Total Size: 295 kB

Filename Size Change
./packages/wiz-ui-next/dist/style.css 11.6 kB -6 B (0%)
./packages/wiz-ui-next/dist/wiz-ui.es.js 75.8 kB +72 B (0%)
./packages/wiz-ui-next/dist/wiz-ui.umd.js 66.6 kB +64 B (0%)
./packages/wiz-ui-react/dist/style.css 11.3 kB -9 B (0%)
./packages/wiz-ui-react/dist/wiz-ui.es.js 69 kB +144 B (0%)
./packages/wiz-ui-react/dist/wiz-ui.umd.js 60.6 kB +127 B (0%)

compressed-size-action

Comment on lines +71 to +76
const inputMarginStyle = () => {
if (checkboxInputMarginY) return checkboxInputMarginY;
if (bordered) return "5px";
return "0px";
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

まず、マージンを変更する機能はチェックボックスの責務ではないので、この機能をチェックボックスに置くのは微妙なんじゃないかなと思っています……!
また、「チェックボックスの位置がズレている」というバグに対して、「チェックボックスの位置がズレていたら利用者側で調整してね」という対応は、プロダクト側のデザイン崩れを修正する場合にこのプロパティを設定しないと直らないので、根本的な解決になっておらず、対策として妥当ではないと考えております……

元々こういったバグはなかったはずなので、別の方法で調整はできるんじゃないかと思っておりまして、なぜこのデザイン崩れが発生したかの理由から解明をお願いできればと思います!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#1139

元々の経緯は複数行になると真ん中に寄せられてしまうケースがあって、
複数行で真ん中によらないよう改善したことが原因でした。

複数行の場合でも上側に寄せる、かつ1行目の文字列の上下真ん中に寄せるという
場合であれば margin を設定して対応できたのですが、

今回の場合は中に<HStack> が中に含まれていて、HStack の余白分も考慮する必要が出てきて
別途で調整する必要が出ました。

外部から style を追加して調整するようにしても、Checkbox の周りに余白ができてしまい、解決に至らなかったので、上記のPR をあまり変えない方針で作業を進めたので 今回はこの margin を props で調整するように実装しました。

Copy link
Collaborator Author

@okakyo okakyo Jan 30, 2024

Choose a reason for hiding this comment

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

そもそも論として、Checkbox のコンポーネント内に子コンポーネントを入れるのを想定されてないかもしれないです。代替案として、

  • Checkbox の ラベル文: label props で設定する
  • Search Input: label を空白にして、横側に HStack で実装するようにする。その際、子コンポーネントをCheckbox のコンポーネントに入れないようにする

この方針で進めたほうがいいかもしれないです。

Copy link
Collaborator

Choose a reason for hiding this comment

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

checkboxの中にコンポーネントが入る設計はあまり崩したくなく、というのも、propsでlabel内に入る文字を制御しようとすると、複数行の表示の方法で工夫が必要になるので、そのインターフェースは保持したいですね……!

すごい安直な方法かもしれませんが、例えばcheckbox-newのtemplateを以下のように変更しまして、

<template>
  <label :class="labelClass">
    <!-- チェックボックスの本体をspanでラップして、inline-flexで上下を揃える -->
    <span
      :style="{
        display: 'inline-flex',
        alignItems: 'center',
      }"
    >
      <input
        type="checkbox"
        :class="[styles.inputStyle, styles.inputMarginStyle]"
        :checked="actualChecked"
        :value="value"
        :id="id"
        :name="name"
        :disabled="disabled"
        @change="handleChange"
      />
      <div :class="[styles.iconWrapperStyle, styles.inputMarginStyle]">
        <div :class="styles.iconPositionStyle">
          <WizIcon :icon="WizICheckBold" color="white.800" size="md" />
        </div>
      </div>
    </span>
    <slot />
  </label>
</template>

checkbox-new.css.tsのinputMarginStyleのmarginを0にすると、

export const inputMarginStyle = style({
  margin: "0",
});

これで上下の中央寄せを保ちつつ、チェックボックスの位置がずれは解消できそうなのですが、これによる懸念点ってありそうでしょうか……?

スクリーンショット 2024-01-31 9 55 08 スクリーンショット 2024-01-31 9 56 18 スクリーンショット 2024-01-31 9 56 39

Copy link
Collaborator Author

@okakyo okakyo Feb 3, 2024

Choose a reason for hiding this comment

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

上記の件だと、複数行になってる場合でも上下中央寄せになっていて、#1139 の問題が再び発生してしまいます。

複数行のケースについて上下真ん中に戻すならこれでいいと思うのですが、戻すべきかどうかについて一度確認してもらったほうがいいかなと思います。こちらの仕様についてご確認のほど、よろしくお願いします 🙇🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

すみませんその要件完全に忘れていました……! 🙇
ちょっと考えてみます!

Copy link
Collaborator

@ichi-h ichi-h Feb 5, 2024

Choose a reason for hiding this comment

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

@okakyo
この問題はcheckboxの問題というより、search-inputのline-heightの問題っぽいですね……
search-inputのラベル内のline-height: 1.75remを外してみたのですが、そうすると問題が解決しますね

line-height有効
スクリーンショット 2024-02-05 14 22 53

line-height無効
スクリーンショット 2024-02-05 14 22 39

つまり問題点は、line-heightが当てられたコンポーネントをslot/childrenに突っ込まれると、そのサイズにcheckboxが対応できず、表示位置がズレたように見える、ですね

なので根本の解決策は、

  • checkboxのslot/childrenにline-heightが指定されたものを突っ込まないようにする
  • checkboxの引数でline-heightを受け付けて、その値を使って表示がずれないように調整のロジックを作る(できるのか……?)

の2つがありそうです

デザインも関わってくるので調整が必要な場所もありそうですが、ひとまずsearch-input内のline-heightが問題なことは特定できたので、search-inputをline-heightを使わずにfigmaのデザインに沿った形で改修できないかの調査をお願いしてもよろしいでしょうか……?
(search-inputの各オプションの改行なし状態の高さがfigmaと揃っていればOKだと思います……!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@okakyo
この問題、checkboxではないことが判明したので、別issueを切っております!
今週は別の優先度の高いタスクがあるので、こちらは一旦後回しで大丈夫です!
#1210

@okakyo
Copy link
Collaborator Author

okakyo commented Feb 24, 2024

#1224 にて、こちらの対応を行った

@ichi-h
Copy link
Collaborator

ichi-h commented Feb 28, 2024

#1210 の解決によりこちらcloseします! 🙇

@ichi-h ichi-h closed this Feb 28, 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.

Bug(checkbox-new, search-input): チェックボックスの位置がズレている
2 participants