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

Remove legacy "checkbox" settings option #29282

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Remove legacy "checkbox" settings option #29282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8a9eb35
Use EditInPlace for identity server picker.
Half-Shot Feb 17, 2025
5aca14d
Update test
Half-Shot Feb 17, 2025
ef4d9ea
Add a test for setting an ID server.
Half-Shot Feb 17, 2025
956d936
fix tests
Half-Shot Feb 17, 2025
206304b
Reformat other :not sections
Half-Shot Feb 18, 2025
03da3b5
forgot a comma
Half-Shot Feb 18, 2025
ffedde2
Update Apperance settings to use toggle switches.
Half-Shot Feb 17, 2025
51cb4a5
Remove unused checkbox setting.
Half-Shot Feb 17, 2025
88e5260
Remove unused import.
Half-Shot Feb 17, 2025
a413ae3
Update tests
Half-Shot Feb 17, 2025
b4c926f
lint
Half-Shot Feb 17, 2025
57e8e51
update apperance screenshot
Half-Shot Feb 25, 2025
a3f1a8c
Merge branch 'develop' into hs/remove-legacy-setting-toggle
Half-Shot Feb 25, 2025
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
4 changes: 2 additions & 2 deletions playwright/e2e/settings/appearance-user-settings-tab/appearance-user-settings-tab.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ test.describe("Appearance user settings tab", () => {
// Click "Show advanced" link button
await tab.getByRole("button", { name: "Show advanced" }).click();

await tab.locator(".mx_Checkbox", { hasText: "Use bundled emoji font" }).click();
await tab.locator(".mx_Checkbox", { hasText: "Use a system font" }).click();
await tab.getByRole("switch", { name: "Use bundled emoji font" }).click();
await tab.getByRole("switch", { name: "Use a system font" }).click();

// Assert that the font-family value was removed
await expect(page.locator("body")).toHaveCSS("font-family", '""');
Expand Down
Binary file modified ...user-settings-tab/appearance-user-settings-tab.spec.ts/appearance-tab-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions res/css/views/settings/tabs/user/_AppearanceUserSettingsTab.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ Please see LICENSE files in the repository root for full details.

.mx_Field.mx_AppearanceUserSettingsTab_checkboxControlledField {
width: 256px;
/* matches checkbox box + padding to align with checkbox label */
margin-inline-start: calc($font-16px + 10px);
/* Line up with Settings field toggle button */
margin-inline-start: 0;
}
5 changes: 1 addition & 4 deletions src/components/views/elements/Field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ interface IProps {
// If specified, contents will appear as a tooltip on the element and
// validation feedback tooltips will be suppressed.
tooltipContent?: JSX.Element | string;
// If specified the tooltip will be shown regardless of feedback
forceTooltipVisible?: boolean;
// If specified, the tooltip with be aligned accorindly with the field, defaults to Right.
tooltipAlignment?: ComponentProps<typeof Tooltip>["placement"];
// If specified alongside tooltipContent, the class name to apply to the
Expand Down Expand Up @@ -274,7 +272,6 @@ export default class Field extends React.PureComponent<PropShapes, IState> {
validateOnChange,
validateOnFocus,
usePlaceholderAsHint,
forceTooltipVisible,
tooltipAlignment,
...inputProps
} = this.props;
Expand All @@ -283,7 +280,7 @@ export default class Field extends React.PureComponent<PropShapes, IState> {
const tooltipProps: Pick<React.ComponentProps<typeof Tooltip>, "aria-live" | "aria-atomic"> = {};
let tooltipOpen = false;
if (tooltipContent || this.state.feedback) {
tooltipOpen = (this.state.focused && forceTooltipVisible) || this.state.feedbackVisible;
tooltipOpen = this.state.feedbackVisible;

if (!tooltipContent) {
tooltipProps["aria-atomic"] = "true";
Expand Down
76 changes: 29 additions & 47 deletions src/components/views/elements/SettingsFlag.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { secureRandomString } from "matrix-js-sdk/src/randomstring";
import SettingsStore from "../../../settings/SettingsStore";
import { _t } from "../../../languageHandler";
import ToggleSwitch from "./ToggleSwitch";
import StyledCheckbox from "./StyledCheckbox";
import { type SettingLevel } from "../../../settings/SettingLevel";
import { type BooleanSettingKey, defaultWatchManager } from "../../../settings/Settings";

Expand All @@ -24,8 +23,6 @@ interface IProps {
roomId?: string; // for per-room settings
label?: string;
isExplicit?: boolean;
// XXX: once design replaces all toggles make this the default
useCheckbox?: boolean;
Comment on lines -27 to -28
Copy link
Member

Choose a reason for hiding this comment

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

This seems to suggest moving everything from toggles to checkboxes, what changed?

Copy link
Member Author

@Half-Shot Half-Shot Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

image

I get the feeling this was a choice made a quite a few design iterations ago. I'm assuming since the only bit of settings to ever use checkboxes was the author's own appearance settings. it wasn't ever expanded to the rest of the components.

If the plan is to still change over to checkboxes, we can check with design. However going one way or the other would reduce some overhead here.

Copy link
Member

@t3chguy t3chguy Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Compound specifically supports both checkboxes and toggles so we should definitely query design which we should favour for what. @element-hq/design

hideIfCannotSet?: boolean;
onChange?(checked: boolean): void;
}
Expand Down Expand Up @@ -80,10 +77,6 @@ export default class SettingsFlag extends React.Component<IProps, IState> {
this.props.onChange?.(checked);
};

private checkBoxOnChange = (e: React.ChangeEvent<HTMLInputElement>): void => {
this.onChange(e.target.checked);
};

private save = async (val?: boolean): Promise<void> => {
await SettingsStore.setValue(
this.props.name,
Expand All @@ -101,45 +94,34 @@ export default class SettingsFlag extends React.Component<IProps, IState> {
const label = this.props.label ?? SettingsStore.getDisplayName(this.props.name, this.props.level);
const description = SettingsStore.getDescription(this.props.name);
const shouldWarn = SettingsStore.shouldHaveWarning(this.props.name);

if (this.props.useCheckbox) {
return (
<StyledCheckbox checked={this.state.value} onChange={this.checkBoxOnChange} disabled={disabled}>
{label}
</StyledCheckbox>
);
} else {
return (
<div className="mx_SettingsFlag">
<label className="mx_SettingsFlag_label" htmlFor={this.id}>
<span className="mx_SettingsFlag_labelText">{label}</span>
{description && (
<div className="mx_SettingsFlag_microcopy">
{shouldWarn
? _t(
"settings|warning",
{},
{
w: (sub) => (
<span className="mx_SettingsTab_microcopy_warning">{sub}</span>
),
description,
},
)
: description}
</div>
)}
</label>
<ToggleSwitch
id={this.id}
checked={this.state.value}
onChange={this.onChange}
disabled={disabled}
tooltip={disabled ? SettingsStore.disabledMessage(this.props.name) : undefined}
title={label ?? undefined}
/>
</div>
);
}
return (
<div className="mx_SettingsFlag">
<label className="mx_SettingsFlag_label" htmlFor={this.id}>
<span className="mx_SettingsFlag_labelText">{label}</span>
{description && (
<div className="mx_SettingsFlag_microcopy">
{shouldWarn
? _t(
"settings|warning",
{},
{
w: (sub) => <span className="mx_SettingsTab_microcopy_warning">{sub}</span>,
description,
},
)
: description}
</div>
)}
</label>
<ToggleSwitch
id={this.id}
checked={this.state.value}
onChange={this.onChange}
disabled={disabled}
tooltip={disabled ? SettingsStore.disabledMessage(this.props.name) : undefined}
title={label ?? undefined}
/>
</div>
);
}
}
9 changes: 1 addition & 8 deletions src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import React, { type ChangeEvent, type ReactNode } from "react";
import { type EmptyObject } from "matrix-js-sdk/src/matrix";

import { _t } from "../../../../../languageHandler";
import SdkConfig from "../../../../../SdkConfig";
import SettingsStore from "../../../../../settings/SettingsStore";
import SettingsFlag from "../../../elements/SettingsFlag";
import Field from "../../../elements/Field";
Expand Down Expand Up @@ -48,7 +47,6 @@ export default class AppearanceUserSettingsTab extends React.Component<EmptyObje
private renderAdvancedSection(): ReactNode {
if (!SettingsStore.getValue(UIFeature.AdvancedSettings)) return null;

const brand = SdkConfig.get().brand;
const toggle = (
<AccessibleButton
kind="link"
Expand All @@ -62,21 +60,18 @@ export default class AppearanceUserSettingsTab extends React.Component<EmptyObje
let advanced: React.ReactNode;

if (this.state.showAdvanced) {
const tooltipContent = _t("settings|appearance|custom_font_description", { brand });
advanced = (
<>
<SettingsFlag name="useCompactLayout" level={SettingLevel.DEVICE} useCheckbox={true} />
<SettingsFlag name="useCompactLayout" level={SettingLevel.DEVICE} />

<SettingsFlag
name="useBundledEmojiFont"
level={SettingLevel.DEVICE}
useCheckbox={true}
onChange={(checked) => this.setState({ useBundledEmojiFont: checked })}
/>
<SettingsFlag
name="useSystemFont"
level={SettingLevel.DEVICE}
useCheckbox={true}
onChange={(checked) => this.setState({ useSystemFont: checked })}
/>
<Field
Expand All @@ -89,8 +84,6 @@ export default class AppearanceUserSettingsTab extends React.Component<EmptyObje

SettingsStore.setValue("systemFont", null, SettingLevel.DEVICE, value.target.value);
}}
tooltipContent={tooltipContent}
forceTooltipVisible={true}
disabled={!this.state.useSystemFont}
value={this.state.systemFont}
/>
Expand Down
4 changes: 4 additions & 0 deletions src/settings/Settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,6 +906,10 @@ export const SETTINGS: Settings = {
default: false,
displayName: _td("settings|appearance|custom_font"),
controller: new SystemFontController(),
description: () =>
_t("settings|appearance|custom_font_description", {
brand: SdkConfig.get().brand,
}),
},
"systemFont": {
supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS,
Expand Down
Loading