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

fix: Fix Switch not toggling. #985

Merged
merged 1 commit into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion src/inputs/Switch.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ type SwitchWrapperProps = Omit<SwitchProps, "onChange" | "selected"> &

function SwitchWrapper({ isHovered, isFocused, ...props }: SwitchWrapperProps) {
const [selected, setSelected] = useState<boolean>(props.selected || false);

return (
<div
css={{
Expand Down
41 changes: 41 additions & 0 deletions src/inputs/Switch.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { useState } from "react";
import { Switch as SwitchComponent, SwitchProps } from "src/inputs";
import { click, render } from "src/utils/rtl";

describe("Switch", () => {
it("can change", async () => {
const onChange = jest.fn();
// Given a switch
const r = await render(<SwitchTest label="Age" onChange={onChange} />);
// Then it defaults no checked
expect(r.age).not.toBeChecked();
// And when we click it, it flips to checked
click(r.age);
expect(r.age).toBeChecked();
expect(onChange).toHaveBeenCalledTimes(1);
// And if we click it again, it flips back to unchecked
click(r.age);
expect(r.age).not.toBeChecked();
expect(onChange).toHaveBeenCalledTimes(2);
});
});

type SwitchTestProps = Omit<SwitchProps, "onChange" | "selected"> & {
onChange?: (value: boolean) => void;
selected?: boolean;
};

function SwitchTest({ selected: initSelected, onChange: _onChange, ...props }: SwitchTestProps) {
const [selected, setSelected] = useState(initSelected || false);
return (
<SwitchComponent
labelStyle="inline"
selected={selected}
onChange={(value) => {
_onChange?.(value);
setSelected(value);
}}
{...props}
/>
);
}
36 changes: 20 additions & 16 deletions src/inputs/Switch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { resolveTooltip } from "src/components";
import { Label } from "src/components/Label";
import { Css, Palette } from "src/Css";
import { Icon } from "../components/Icon";
import { toToggleState } from "../utils";
import { toToggleState, useTestIds } from "../utils";

export interface SwitchProps {
/** Whether the element should receive focus on render. */
Expand All @@ -17,7 +17,7 @@ export interface SwitchProps {
label: string;
/** Where to put the label. */
labelStyle?: "form" | "inline" | "filter" | "hidden" | "left" | "centered"; // TODO: Update `labelStyle` to make consistent with other `labelStyle` properties in the library
/** Whether or not to hide the label */
/** Whether to hide the label */
hideLabel?: boolean;
/** Handler when the interactive element state changes. */
onChange: (value: boolean) => void;
Expand Down Expand Up @@ -49,12 +49,13 @@ export function Switch(props: SwitchProps) {
const { isFocusVisible: isKeyboardFocus, focusProps } = useFocusRing(otherProps);
const { hoverProps, isHovered } = useHover(ariaProps);
const tooltip = resolveTooltip(disabled, props.tooltip);
const tid = useTestIds(otherProps, label);

return (
<div
<label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A previous styling change changed this top-level element from a label to a div, but that seems to have broken the input-flips-to-true/false because the input isn't wrapped in a label anymore.

This is a little janky b/c we now have two labels in this component, but the tests/stories show that this fixes the regression, so I'm prioritizing getting the fix out vs. a perfect DOM structure that doesn't have duplicate labels.

{...hoverProps}
css={{
...Css.relative.cursorPointer.df.w("max-content").selectNone.$,
...Css.relative.cursorPointer.df.wmaxc.selectNone.$,
...(labelStyle === "form" && Css.fdc.$),
...(labelStyle === "left" && Css.w100.aic.$),
...(labelStyle === "inline" && Css.gap2.aic.$),
Expand Down Expand Up @@ -105,9 +106,9 @@ export function Switch(props: SwitchProps) {
<Label label={label} tooltip={tooltip} inline xss={Css.smMd.gray900.if(compact).add("lineHeight", "1").$} />
)}
<VisuallyHidden>
<input ref={ref} {...inputProps} {...focusProps} />
<input ref={ref} {...inputProps} {...focusProps} {...tid} />
</VisuallyHidden>
</div>
</label>
);
}

Expand All @@ -122,22 +123,25 @@ export const switchFocusStyles = Css.bshFocus.$;
export const switchSelectedHoverStyles = Css.bgBlue900.$;

// Circle inside Switcher/Toggle element styles
const switchCircleDefaultStyles = (isCompact: boolean) => ({
...Css.wPx(circleDiameter(isCompact))
.hPx(circleDiameter(isCompact))
.br100.bgWhite.bshBasic.absolute.leftPx(2)
.topPx(2).transition.df.aic.jcc.$,
svg: Css.hPx(toggleHeight(isCompact) / 2).wPx(toggleHeight(isCompact) / 2).$,
});
function switchCircleDefaultStyles(isCompact: boolean) {
return {
...Css.wPx(circleDiameter(isCompact))
.hPx(circleDiameter(isCompact))
.br100.bgWhite.bshBasic.absolute.leftPx(2)
.topPx(2).transition.df.aic.jcc.$,
svg: Css.hPx(toggleHeight(isCompact) / 2).wPx(toggleHeight(isCompact) / 2).$,
};
}

/**
* Affecting the `left` property due to transitions only working when there is
* a previous value to work from.
*
* Calculation is as follow:
* Calculation is as follows:
* - `100%` is the toggle width
* - `${circleDiameter(isCompact)}px` is the circle diameter
* - `2px` is to keep 2px edge spacing.
*/
const switchCircleSelectedStyles = (isCompact: boolean) =>
Css.left(`calc(100% - ${circleDiameter(isCompact)}px - 2px);`).$;
function switchCircleSelectedStyles(isCompact: boolean) {
return Css.left(`calc(100% - ${circleDiameter(isCompact)}px - 2px);`).$;
}
Loading