Use semantic headings in user settings - Notifications (#10948)

* use semantic headings in user notif settings * unset margin for subsection content when no heading * remove debug
2023-05-24 11:36:09 +12:00 · 2023-05-24 11:36:09 +12:00 · 9211da20f4
commit 9211da20f4
parent f491f2f951
6 changed files with 81 additions and 85 deletions
--- a/res/css/components/views/settings/shared/_SettingsSubsection.pcss
+++ b/res/css/components/views/settings/shared/_SettingsSubsection.pcss
@ -52,4 +52,8 @@ limitations under the License.
    &.mx_SettingsSubsection_contentStretch {
        justify-items: stretch;
    }
+
+    &.mx_SettingsSubsection_noHeading {
+        margin-top: 0;
+    }
 }
--- a/res/css/views/settings/_Notifications.pcss
+++ b/res/css/views/settings/_Notifications.pcss
@ -20,7 +20,6 @@ limitations under the License.
    grid-template-columns: auto repeat(3, 62px);
    place-items: center center;
    grid-gap: 8px;
-    margin-top: $spacing-40;

    /* Override StyledRadioButton default styles */
    .mx_StyledRadioButton {
@ -34,6 +33,11 @@ limitations under the License.
            display: none;
        }
    }
+
+    // left align section heading
+    .mx_SettingsSubsectionHeading {
+        justify-self: start;
+    }
 }

 .mx_UserNotifSettings_gridRowContainer {
@ -51,10 +55,6 @@ limitations under the License.
    /* force it inline using float */
    float: left;
 }
-.mx_UserNotifSettings_gridRowHeading {
-    font-size: $font-18px;
-    font-weight: var(--font-semi-bold);
-}

 .mx_UserNotifSettings_gridColumnLabel {
    color: $secondary-content;
@ -70,39 +70,35 @@ limitations under the License.
    margin-top: -$spacing-4;
 }

-.mx_UserNotifSettings {
-    color: $primary-content; /* override from default settings page styles */
+.mx_UserNotifSettings_floatingSection {
+    margin-top: 40px;

-    .mx_UserNotifSettings_floatingSection {
-        margin-top: 40px;
-
-        & > div:first-child {
-            /* section header */
-            font-size: $font-18px;
-            font-weight: var(--font-semi-bold);
-        }
-
-        > table {
-            border-collapse: collapse;
-            border-spacing: 0;
-            margin-top: 8px;
-
-            tr > td:first-child {
-                /* Just for a bit of spacing */
-                padding-right: 8px;
-            }
-        }
+    & > div:first-child {
+        /* section header */
+        font-size: $font-18px;
+        font-weight: var(--font-semi-bold);
    }

-    .mx_UserNotifSettings_clearNotifsButton {
+    > table {
+        border-collapse: collapse;
+        border-spacing: 0;
        margin-top: 8px;
-    }

-    .mx_TagComposer {
-        margin-top: 35px; /* lots of distance from the last line of the table */
+        tr > td:first-child {
+            /* Just for a bit of spacing */
+            padding-right: 8px;
+        }
    }
 }

+.mx_UserNotifSettings_clearNotifsButton {
+    margin-top: 8px;
+}
+
+.mx_TagComposer {
+    margin-top: 35px; /* lots of distance from the last line of the table */
+}
+
 .mx_AccessibleButton.mx_NotificationSound_browse {
    margin-right: 10px;
 }
--- a/src/components/views/settings/Notifications.tsx
+++ b/src/components/views/settings/Notifications.tsx
@ -48,6 +48,8 @@ import {
    updatePushRuleActions,
 } from "../../../utils/pushRules/updatePushRuleActions";
 import { Caption } from "../typography/Caption";
+import { SettingsSubsectionHeading } from "./shared/SettingsSubsectionHeading";
+import SettingsSubsection from "./shared/SettingsSubsection";

 // TODO: this "view" component still has far too much application logic in it,
 // which should be factored out to other files.
@ -649,16 +651,14 @@ export default class Notifications extends React.PureComponent<IProps, IState> {

    private renderTopSection(): JSX.Element {
        const masterSwitch = (
-            <>
-                <LabelledToggleSwitch
-                    data-testid="notif-master-switch"
-                    value={!this.isInhibited}
-                    label={_t("Enable notifications for this account")}
-                    caption={_t("Turn off to disable notifications on all your devices and sessions")}
-                    onChange={this.onMasterRuleChanged}
-                    disabled={this.state.phase === Phase.Persisting}
-                />
-            </>
+            <LabelledToggleSwitch
+                data-testid="notif-master-switch"
+                value={!this.isInhibited}
+                label={_t("Enable notifications for this account")}
+                caption={_t("Turn off to disable notifications on all your devices and sessions")}
+                onChange={this.onMasterRuleChanged}
+                disabled={this.state.phase === Phase.Persisting}
+            />
        );

        // If all the rules are inhibited, don't show anything.
@ -680,7 +680,7 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
            ));

        return (
-            <>
+            <SettingsSubsection>
                {masterSwitch}

                <LabelledToggleSwitch
@ -718,7 +718,7 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
                )}

                {emailSwitches}
-            </>
+            </SettingsSubsection>
        );
    }

@ -814,7 +814,7 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
            </fieldset>
        ));

-        let sectionName: TranslatedString;
+        let sectionName: string;
        switch (category) {
            case RuleClass.VectorGlobal:
                sectionName = _t("Global");
@ -830,11 +830,9 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
        }

        return (
-            <>
+            <div>
                <div data-testid={`notif-section-${category}`} className="mx_UserNotifSettings_grid">
-                    <span className="mx_UserNotifSettings_gridRowLabel mx_UserNotifSettings_gridRowHeading">
-                        {sectionName}
-                    </span>
+                    <SettingsSubsectionHeading heading={sectionName} />
                    <span className="mx_UserNotifSettings_gridColumnLabel">{VectorStateToLabel[VectorState.Off]}</span>
                    <span className="mx_UserNotifSettings_gridColumnLabel">{VectorStateToLabel[VectorState.On]}</span>
                    <span className="mx_UserNotifSettings_gridColumnLabel">{VectorStateToLabel[VectorState.Loud]}</span>
@ -842,7 +840,7 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
                </div>
                {clearNotifsButton}
                {keywordComposer}
-            </>
+            </div>
        );
    }

@ -877,13 +875,13 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
        }

        return (
-            <div className="mx_UserNotifSettings">
+            <>
                {this.renderTopSection()}
                {this.renderCategory(RuleClass.VectorGlobal)}
                {this.renderCategory(RuleClass.VectorMentions)}
                {this.renderCategory(RuleClass.VectorOther)}
                {this.renderTargets()}
-            </div>
+            </>
        );
    }
 }
--- a/src/components/views/settings/shared/SettingsSubsection.tsx
+++ b/src/components/views/settings/shared/SettingsSubsection.tsx
@ -20,7 +20,7 @@ import React, { HTMLAttributes } from "react";
 import { SettingsSubsectionHeading } from "./SettingsSubsectionHeading";

 export interface SettingsSubsectionProps extends HTMLAttributes<HTMLDivElement> {
-    heading: string | React.ReactNode;
+    heading?: string | React.ReactNode;
    description?: string | React.ReactNode;
    children?: React.ReactNode;
    // when true content will be justify-items: stretch, which will make items within the section stretch to full width.
@ -50,6 +50,7 @@ export const SettingsSubsection: React.FC<SettingsSubsectionProps> = ({
        <div
            className={classNames("mx_SettingsSubsection_content", {
                mx_SettingsSubsection_contentStretch: !!stretchContent,
+                mx_SettingsSubsection_noHeading: !heading && !description,
            })}
        >
            {children}
--- a/src/components/views/settings/tabs/user/NotificationUserSettingsTab.tsx
+++ b/src/components/views/settings/tabs/user/NotificationUserSettingsTab.tsx
@ -18,16 +18,17 @@ import React from "react";

 import { _t } from "../../../../../languageHandler";
 import Notifications from "../../Notifications";
+import { SettingsSection } from "../../shared/SettingsSection";
+import SettingsTab from "../SettingsTab";

 export default class NotificationUserSettingsTab extends React.Component {
    public render(): React.ReactNode {
        return (
-            <div className="mx_SettingsTab">
-                <div className="mx_SettingsTab_heading">{_t("Notifications")}</div>
-                <div className="mx_SettingsTab_section mx_SettingsTab_subsectionText">
+            <SettingsTab>
+                <SettingsSection heading={_t("Notifications")}>
                    <Notifications />
-                </div>
-            </div>
+                </SettingsSection>
+            </SettingsTab>
        );
    }
 }
--- a/test/components/views/settings/snapshots/Notifications-test.tsx.snap
+++ b/test/components/views/settings/snapshots/Notifications-test.tsx.snap
@ -3,40 +3,36 @@
 exports[`<Notifications /> main notification switches renders only enable notifications switch when notifications are disabled 1`] = `
 <div>
  <div
-    class="mx_UserNotifSettings"
+    class="mx_SettingsFlag"
+    data-testid="notif-master-switch"
  >
-    <div
-      class="mx_SettingsFlag"
-      data-testid="notif-master-switch"
+    <span
+      class="mx_SettingsFlag_label"
    >
-      <span
-        class="mx_SettingsFlag_label"
-      >
-        <div
-          id="mx_LabelledToggleSwitch_testid_0"
-        >
-          Enable notifications for this account
-        </div>
-        <span
-          class="mx_Caption"
-          id="mx_LabelledToggleSwitch_testid_0_caption"
-        >
-          Turn off to disable notifications on all your devices and sessions
-        </span>
-      </span>
      <div
-        aria-checked="false"
-        aria-describedby="mx_LabelledToggleSwitch_testid_0_caption"
-        aria-disabled="false"
-        aria-labelledby="mx_LabelledToggleSwitch_testid_0"
-        class="mx_AccessibleButton mx_ToggleSwitch mx_ToggleSwitch_enabled"
-        role="switch"
-        tabindex="0"
+        id="mx_LabelledToggleSwitch_testid_0"
      >
-        <div
-          class="mx_ToggleSwitch_ball"
-        />
+        Enable notifications for this account
      </div>
+      <span
+        class="mx_Caption"
+        id="mx_LabelledToggleSwitch_testid_0_caption"
+      >
+        Turn off to disable notifications on all your devices and sessions
+      </span>
+    </span>
+    <div
+      aria-checked="false"
+      aria-describedby="mx_LabelledToggleSwitch_testid_0_caption"
+      aria-disabled="false"
+      aria-labelledby="mx_LabelledToggleSwitch_testid_0"
+      class="mx_AccessibleButton mx_ToggleSwitch mx_ToggleSwitch_enabled"
+      role="switch"
+      tabindex="0"
+    >
+      <div
+        class="mx_ToggleSwitch_ball"
+      />
    </div>
  </div>
 </div>