Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Use semantic headings in user settings Labs #10773

Merged
merged 12 commits into from
May 16, 2023
Merged
1 change: 0 additions & 1 deletion res/css/_components.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,6 @@
@import "./views/settings/tabs/user/_GeneralUserSettingsTab.pcss";
@import "./views/settings/tabs/user/_HelpUserSettingsTab.pcss";
@import "./views/settings/tabs/user/_KeyboardUserSettingsTab.pcss";
@import "./views/settings/tabs/user/_LabsUserSettingsTab.pcss";
@import "./views/settings/tabs/user/_MjolnirUserSettingsTab.pcss";
@import "./views/settings/tabs/user/_PreferencesUserSettingsTab.pcss";
@import "./views/settings/tabs/user/_SecurityUserSettingsTab.pcss";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,8 @@ limitations under the License.
margin-bottom: $spacing-8;
}
}

&.mx_SettingsSubsection_contentStretch {
justify-items: stretch;
}
}
5 changes: 0 additions & 5 deletions res/css/views/beta/_BetaCard.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ limitations under the License.
*/

.mx_BetaCard {
margin-bottom: $spacing-20;
richvdh marked this conversation as resolved.
Show resolved Hide resolved
padding: $spacing-24;
background-color: $system;
border-radius: 8px;
Expand Down Expand Up @@ -114,10 +113,6 @@ limitations under the License.
}
}
}

&:last-child {
margin-bottom: 0;
}
}

.mx_BetaCard_betaPill {
Expand Down
1 change: 1 addition & 0 deletions res/css/views/elements/_SettingsFlag.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ limitations under the License.
align-items: flex-start;
justify-content: space-between;
margin-bottom: 4px;
width: 100%;
Copy link
Member

Choose a reason for hiding this comment

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

can you help me understand why this is needed?

Copy link
Contributor

@luixxiul luixxiul May 10, 2023

Choose a reason for hiding this comment

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

Otherwise:

1

The switches align on PreferencesTab without the declaration, though.

So, I am wondering why the declaration is not required for #10794. 603e2f8 sets the value for the tabs.

Copy link
Member

Choose a reason for hiding this comment

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

ok, but this wasn't needed before (was it?): why is it needed now?

Copy link
Contributor

@luixxiul luixxiul May 11, 2023

Choose a reason for hiding this comment

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

Adding justify-items: stretch to mx_SettingsSubsection_content instead might perhaps be more natural for a grid layout.

Edit: why would it not possible to add mx_SettingsSubsection_contentStretch class instead?

Copy link
Member

Choose a reason for hiding this comment

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

@kerryarchibald I still don't understand why this is needed now when it wasn't before. I'm not going to hold this review up any further as we've been pinging it back and forth more than long enough and it's somewhat trivial, but if you do get a chance to record a reason here - or better yet, add a comment to the CSS file - then that would be much appreciated.


.mx_ToggleSwitch {
flex: 0 0 auto;
Expand Down
1 change: 0 additions & 1 deletion res/css/views/settings/tabs/_SettingsTab.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ limitations under the License.
*/

.mx_SettingsTab {
--SettingsTab_section-margin-bottom-preferences-labs: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

It is still used here:

margin-bottom: var(--SettingsTab_section-margin-bottom-preferences-labs);

Context: #8828 (comment)

Copy link
Contributor

@luixxiul luixxiul May 6, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, missed that when I was splitting this into multiple PRs

--SettingsTab_heading_nth_child-margin-top: 30px; /* TODO: Use a spacing variable */
--SettingsTab_fullWidthField-margin-inline-end: 100px;
--SettingsTab_tooltip-max-width: 120px; /* So it fits in the space provided by the page */
Expand Down
27 changes: 0 additions & 27 deletions res/css/views/settings/tabs/user/_LabsUserSettingsTab.pcss

This file was deleted.

19 changes: 17 additions & 2 deletions src/components/views/settings/shared/SettingsSubsection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import classNames from "classnames";
import React, { HTMLAttributes } from "react";

import { SettingsSubsectionHeading } from "./SettingsSubsectionHeading";
Expand All @@ -22,6 +23,8 @@ export interface SettingsSubsectionProps extends HTMLAttributes<HTMLDivElement>
heading: string | React.ReactNode;
description?: string | React.ReactNode;
children?: React.ReactNode;
// when true content will be justify-items: stretch
stretchContent?: boolean;
kerryarchibald marked this conversation as resolved.
Show resolved Hide resolved
}

export const SettingsSubsectionText: React.FC<HTMLAttributes<HTMLDivElement>> = ({ children, ...rest }) => (
Expand All @@ -30,15 +33,27 @@ export const SettingsSubsectionText: React.FC<HTMLAttributes<HTMLDivElement>> =
</div>
);

export const SettingsSubsection: React.FC<SettingsSubsectionProps> = ({ heading, description, children, ...rest }) => (
export const SettingsSubsection: React.FC<SettingsSubsectionProps> = ({
heading,
description,
children,
stretchContent,
...rest
}) => (
<div {...rest} className="mx_SettingsSubsection">
{typeof heading === "string" ? <SettingsSubsectionHeading heading={heading} /> : <>{heading}</>}
{!!description && (
<div className="mx_SettingsSubsection_description">
<SettingsSubsectionText>{description}</SettingsSubsectionText>
</div>
)}
<div className="mx_SettingsSubsection_content">{children}</div>
<div
className={classNames("mx_SettingsSubsection_content", {
mx_SettingsSubsection_contentStretch: stretchContent,
richvdh marked this conversation as resolved.
Show resolved Hide resolved
})}
>
{children}
</div>
</div>
);

Expand Down
8 changes: 4 additions & 4 deletions src/components/views/settings/tabs/SettingsTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
import React from "react";
import React, { HTMLAttributes } from "react";

export interface SettingsTabProps {
export interface SettingsTabProps extends Omit<HTMLAttributes<HTMLDivElement>, "className"> {
children?: React.ReactNode;
}

Expand All @@ -37,8 +37,8 @@ export interface SettingsTabProps {
* </SettingsTab>
* ```
*/
const SettingsTab: React.FC<SettingsTabProps> = ({ children }) => (
<div className="mx_SettingsTab">
const SettingsTab: React.FC<SettingsTabProps> = ({ children, ...rest }) => (
<div {...rest} className="mx_SettingsTab">
richvdh marked this conversation as resolved.
Show resolved Hide resolved
<div className="mx_SettingsTab_sections">{children}</div>
</div>
);
Expand Down
51 changes: 29 additions & 22 deletions src/components/views/settings/tabs/user/LabsUserSettingsTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ import BetaCard from "../../../beta/BetaCard";
import SettingsFlag from "../../../elements/SettingsFlag";
import { LabGroup, labGroupNames } from "../../../../../settings/Settings";
import { EnhancedMap } from "../../../../../utils/maps";
import { SettingsSection } from "../../shared/SettingsSection";
import SettingsSubsection, { SettingsSubsectionText } from "../../shared/SettingsSubsection";
import SettingsTab from "../SettingsTab";

export default class LabsUserSettingsTab extends React.Component<{}> {
private readonly labs: string[];
Expand Down Expand Up @@ -54,11 +57,11 @@ export default class LabsUserSettingsTab extends React.Component<{}> {
let betaSection: JSX.Element | undefined;
if (this.betas.length) {
betaSection = (
<div data-testid="labs-beta-section" className="mx_SettingsTab_section">
<>
{this.betas.map((f) => (
<BetaCard key={f} featureId={f} />
))}
</div>
</>
);
}

Expand Down Expand Up @@ -93,31 +96,35 @@ export default class LabsUserSettingsTab extends React.Component<{}> {
labsSections = (
<>
{sortBy(Array.from(groups.entries()), "0").map(([group, flags]) => (
<div className="mx_SettingsTab_section" key={group} data-testid={`labs-group-${group}`}>
<span className="mx_SettingsTab_subheading">{_t(labGroupNames[group])}</span>
<SettingsSubsection
key={group}
data-testid={`labs-group-${group}`}
heading={_t(labGroupNames[group])}
>
{flags}
</div>
</SettingsSubsection>
))}
</>
);
}

return (
<div className="mx_SettingsTab mx_LabsUserSettingsTab">
<div className="mx_SettingsTab_heading">{_t("Upcoming features")}</div>
<div className="mx_SettingsTab_subsectionText">
{_t(
"What's next for %(brand)s? " +
"Labs are the best way to get things early, " +
"test out new features and help shape them before they actually launch.",
{ brand: SdkConfig.get("brand") },
)}
</div>
{betaSection}
<SettingsTab>
<SettingsSection heading={_t("Upcoming features")}>
<SettingsSubsectionText>
{_t(
"What's next for %(brand)s? " +
"Labs are the best way to get things early, " +
"test out new features and help shape them before they actually launch.",
{ brand: SdkConfig.get("brand") },
)}
</SettingsSubsectionText>
{betaSection}
</SettingsSection>

{labsSections && (
<>
<div className="mx_SettingsTab_heading">{_t("Early previews")}</div>
<div className="mx_SettingsTab_subsectionText">
<SettingsSection heading={_t("Early previews")}>
<SettingsSubsectionText>
{_t(
"Feeling experimental? " +
"Try out our latest ideas in development. " +
Expand All @@ -139,11 +146,11 @@ export default class LabsUserSettingsTab extends React.Component<{}> {
},
},
)}
</div>
</SettingsSubsectionText>
{labsSections}
</>
</SettingsSection>
)}
</div>
</SettingsTab>
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ limitations under the License.
*/

import React from "react";
import { render, waitFor } from "@testing-library/react";
import { render, screen, waitFor } from "@testing-library/react";
import { defer } from "matrix-js-sdk/src/utils";

import LabsUserSettingsTab from "../../../../../../src/components/views/settings/tabs/user/LabsUserSettingsTab";
Expand Down Expand Up @@ -51,26 +51,27 @@ describe("<LabsUserSettingsTab />", () => {
});

it("renders settings marked as beta as beta cards", () => {
const { getByTestId } = render(getComponent());
expect(getByTestId("labs-beta-section")).toMatchSnapshot();
render(getComponent());
expect(screen.getByText("Upcoming features").parentElement!).toMatchSnapshot();
});

it("does not render non-beta labs settings when disabled in config", () => {
const { container } = render(getComponent());
render(getComponent());
expect(sdkConfigSpy).toHaveBeenCalledWith("show_labs_settings");

const labsSections = container.getElementsByClassName("mx_SettingsTab_section");
// only section is beta section
expect(labsSections.length).toEqual(1);
expect(screen.queryByText("Early previews")).not.toBeInTheDocument();
});

it("renders non-beta labs settings when enabled in config", () => {
// enable labs
sdkConfigSpy.mockImplementation((configName) => configName === "show_labs_settings");
const { container } = render(getComponent());

const labsSections = container.getElementsByClassName("mx_SettingsTab_section");
expect(labsSections).toHaveLength(12);
// non-beta labs section
expect(screen.getByText("Early previews")).toBeInTheDocument();
const labsSections = container.getElementsByClassName("mx_SettingsSubsection");
expect(labsSections).toHaveLength(11);
});

it("allow setting a labs flag which requires unstable support once support is confirmed", async () => {
Expand Down
Loading