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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5643fd
[#1196] CheckboxNew に marginYの位置を調整する props 追加
okakyo Jan 27, 2024
0884005
[#1196] SearchInput の checkbox の位置を修正
okakyo Jan 27, 2024
5c11dd5
[#1196] add changeset
okakyo Jan 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/lazy-deers-dream.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@wizleap-inc/wiz-ui-react": minor
"@wizleap-inc/wiz-ui-next": minor
"@wizleap-inc/wiz-ui-styles": minor
---

[#1196] Checkbox の位置を調整するための Props 追加
9 changes: 0 additions & 9 deletions packages/styles/bases/checkbox-new.css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,6 @@ const inputShapeStyle = style({
borderRadius: "2px",
});

export const inputMarginStyle = styleVariants({
default: {
margin: "0.05rem 0",
},
bordered: {
margin: "0.25rem 0",
},
});

export const inputStyle = style([
inputShapeStyle,
{
Expand Down
15 changes: 10 additions & 5 deletions packages/wiz-ui-next/src/components/base/inputs/checkbox-new/checkbox-new.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,18 @@
<label :class="labelClass">
<input
type="checkbox"
:class="[styles.inputStyle, styles.inputMarginStyle[borderState]]"
:class="[styles.inputStyle]"
:checked="actualChecked"
:value="value"
:id="id"
:name="name"
:disabled="disabled"
@change="handleChange"
:style="{ margin: `${checkboxInputMargin} 0` }"
/>
<div
:class="[styles.iconWrapperStyle, styles.inputMarginStyle[borderState]]"
:class="[styles.iconWrapperStyle]"
:style="{ margin: `${checkboxInputMargin} 0` }"
>
<div :class="styles.iconPositionStyle">
<WizIcon :icon="WizICheckBold" color="white.800" size="md" />
Expand All @@ -36,6 +38,7 @@ interface Props {
strikeThrough?: boolean;
bordered?: boolean;
error?: boolean;
checkboxInputMarginY?: string;
}
const props = defineProps<Props>();

Expand Down Expand Up @@ -81,7 +84,9 @@ const handleChange = (e: Event) => {
}
};

const borderState: "bordered" | "default" = props.bordered
? "bordered"
: "default";
const checkboxInputMargin = computed(() => {
if (props.checkboxInputMarginY) return props.checkboxInputMarginY;
if (props.bordered) return "5px";
return "0px";
});
</script>
1 change: 1 addition & 0 deletions packages/wiz-ui-next/src/components/base/inputs/search-input/search-input.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
:checked="checkValues.includes(item.value)"
:value="item.value"
:id="`${item.label}_${item.value}`"
checkbox-input-margin-y="5px"
@update:checked="handleClickCheckbox(item.value)"
>
<WizHStack width="100%" align="center" nowrap gap="xs2">
Expand Down
1 change: 1 addition & 0 deletions packages/wiz-ui-next/src/components/base/inputs/search-input/search-popup.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
:checked="checkValues.includes(item.value)"
:value="item.value"
:id="`${item.label}_${item.value}`"
checkbox-input-margin-y="5px"
@update:checked="handleClickCheckbox(item.value)"
>
<WizHStack width="100%" align="center" nowrap gap="xs2">
Expand Down
24 changes: 16 additions & 8 deletions packages/wiz-ui-react/src/components/base/inputs/checkbox-new/components/checkbox-new.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type Props = BaseProps & {
bordered?: boolean;
error?: boolean;
children?: ReactNode;
checkboxInputMarginY?: string;
onChange?: (e: ChangeEvent<HTMLInputElement>) => void;
};

Expand All @@ -41,6 +42,7 @@ const CheckboxNew = forwardRef<HTMLInputElement, Props>(
children,
strikeThrough,
bordered,
checkboxInputMarginY,
error,
onChange,
...props
Expand All @@ -66,6 +68,12 @@ const CheckboxNew = forwardRef<HTMLInputElement, Props>(
[isControlled, onChange]
);

const inputMarginStyle = () => {
if (checkboxInputMarginY) return checkboxInputMarginY;
if (bordered) return "5px";
return "0px";
};

Comment on lines +71 to +76
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

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

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

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

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

Copy link
Collaborator Author

@okakyo okakyo Jan 30, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

const labelClassName = useMemo(() => {
const borderedState = (() => {
if (props.disabled) {
Expand Down Expand Up @@ -101,18 +109,18 @@ const CheckboxNew = forwardRef<HTMLInputElement, Props>(
{...props}
ref={ref}
type="checkbox"
className={clsx(
styles.inputStyle,
styles.inputMarginStyle[bordered ? "bordered" : "default"]
)}
className={clsx(styles.inputStyle)}
checked={actualChecked}
onChange={handleChange}
style={{
margin: `${inputMarginStyle()} 0px`,
}}
/>
<div
className={clsx(
styles.iconWrapperStyle,
styles.inputMarginStyle[bordered ? "bordered" : "default"]
)}
className={clsx(styles.iconWrapperStyle)}
style={{
margin: `${inputMarginStyle()} 0px`,
}}
>
{actualChecked && (
<div className={styles.iconPositionStyle}>
Expand Down
1 change: 1 addition & 0 deletions ...es/wiz-ui-react/src/components/base/inputs/search-input/components/search-popup-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ export const SearchPopupPanel: FC<Props> = ({
value={option.value}
id={`${option.label}-${option.value}`}
checked={values.includes(option.value)}
checkboxInputMarginY="5px"
onChange={(e) => {
handleChangeValues(option.value, e.target.checked);
}}
Expand Down
Loading