Repository: manusa/electronim
Pull request #219 (2022-11-20 08:36:01)

✨ settings global-notifications toggle uses Material 3 switch

sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=manusa_electronim&pullRequest=219)

[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=BUG)  
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=VULNERABILITY)  
[![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=manusa_electronim&pullRequest=219&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=manusa_electronim&pullRequest=219&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=manusa_electronim&pullRequest=219&resolved=false&types=SECURITY_HOTSPOT)  
[![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=manusa_electronim&pullRequest=219&resolved=false&types=CODE_SMELL)

[![85.7%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60-16px.png '85.7%')](https://sonarcloud.io/component_measures?id=manusa_electronim&pullRequest=219&metric=new_coverage&view=list) [85.7% Coverage](https://sonarcloud.io/component_measures?id=manusa_electronim&pullRequest=219&metric=new_coverage&view=list)  
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=manusa_electronim&pullRequest=219&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=manusa_electronim&pullRequest=219&metric=new_duplicated_lines_density&view=list)



Patch:
diff --git a/src/settings/settings.browser.css b/src/settings/settings.browser.css
index 0f5192a..b4be249 100644
--- a/src/settings/settings.browser.css
+++ b/src/settings/settings.browser.css
@@ -53,13 +53,11 @@
     position: relative;
     transform: scaleY(0);
     height: 0;
-    --x-margin: calc(-1 * var(--material3-card-padding));
-    margin: 0 var(--x-margin);
+    margin: 0;
     transition:
         margin-top 0.3s  ease-out,
         height 0.3s  ease-out,
         transform 0.3s ease-out;
-    overflow: hidden;
 }
 
 .settings__tab--expanded .settings__tab-advanced {
@@ -81,23 +79,30 @@
     flex: 1;
 }
 
-.settings__tab .settings__option {
-    display: flex;
-    align-items: center;
-    gap: 6px;
-    height: 56px;
-    padding: 0 16px;
+.settings__option {
+    --x-margin: calc(-1 * var(--material3-card-padding));
+    margin: 0 var(--x-margin);
+    padding: 0;
     font-family: var(--md-sys-typescale-body-large-font);
     font-size: var(--md-sys-typescale-body-large-size);
-    line-height: var(--md-sys-typescale-body-large-line-height);
     user-select: none;
     cursor: pointer;
 }
-.settings__tab .settings__option:hover {
+
+.settings__option-content {
+    display: flex;
+    align-items: center;
+    line-height: var(--md-sys-typescale-body-large-line-height);
+    height: 56px;
+    gap: 6px;
+    padding: 0 var(--material3-card-padding);
+}
+
+.settings__option-content:hover {
     background-color: var(--md-sys-color-surface-variant);
 }
 
-.settings__tab .settings__option-label {
+.settings__option .settings__option-label {
     flex: 1;
 }
 
diff --git a/src/settings/settings.common.browser.mjs b/src/settings/settings.common.browser.mjs
index fa96ddb..0cd334d 100644
--- a/src/settings/settings.common.browser.mjs
+++ b/src/settings/settings.common.browser.mjs
@@ -13,6 +13,7 @@
    See the License for the specific language governing permissions and
    limitations under the License.
  */
+import {html, Icon, Switch} from '../components/index.mjs';
 
 export const prependProtocol = url => {
   if (url && !url.match(/^https?:\/\/.+/)) {
@@ -40,3 +41,19 @@ export const newId = () => (
   new Date().getTime().toString(36) + Math.random().toString(36).slice(2) // NOSONAR
 );
 
+export const SettingsOption = ({
+  onClick,
+  checked,
+  icon,
+  label,
+  title,
+  className = ''
+}) => html`
+  <div class=${`settings__option ${className}`} onClick=${onClick} title=${title}>
+    <div class='settings__option-content'>
+      <${Icon}>${icon}</${Icon}>
+      <span class='settings__option-label'>${label}</span>
+      <${Switch} checked=${checked} onClick=${onClick} title=${title} />
+    </div>
+  </div>
+`;
diff --git a/src/settings/settings.other.browser.mjs b/src/settings/settings.other.browser.mjs
index 476874f..9c5851f 100644
--- a/src/settings/settings.other.browser.mjs
+++ b/src/settings/settings.other.browser.mjs
@@ -13,42 +13,47 @@
    See the License for the specific language governing permissions and
    limitations under the License.
  */
-import {ELECTRONIM_VERSION, Card, Checkbox, HorizontalField, Select, html} from '../components/index.mjs';
+import {ELECTRONIM_VERSION, html, Card, HorizontalField, Icon, Select} from '../components/index.mjs';
 import {
   isPaneActive,
   setTheme,
   toggleNotifications
 } from './settings.reducer.browser.mjs';
+import {SettingsOption} from './settings.common.browser.mjs';
 
-export const OtherPane = ({dispatch, state}) => isPaneActive(state)(OtherPane.id) && html`
-  <h2 class='title'>Other</h2>
-  <${Card} className='settings__other'>
-    <div style=${{display: 'flex', justifyContent: 'flex-start' /* eslint-disable no-warning-comments *//*
-    TODO: remove when Bulma is out
-    */}}>
-      <${HorizontalField} label='Theme' data-testid='settings-theme-select'>
-        <${Select} value=${state.theme} onChange=${e => setTheme({dispatch})(e.target.value)}>
-          <${Select.Option} value='system'>system</${Select.Option}>
-          <${Select.Option} value='light'>light</${Select.Option}>
-          <${Select.Option} value='dark'>dark</${Select.Option}>
-        </${Select}>
-      </${HorizontalField}>
-    </div>
-    <${Card.Divider} />
-    <div class="settings__global-notifications container">
-      <${Checkbox}
-        label="Disable notifications globally"
-        icon=${state.disableNotificationsGlobally ? 'fa-bell-slash' : 'fa-bell'}
-        checked=${state.disableNotificationsGlobally}
-        value=${state.disableNotificationsGlobally}
-        onClick=${toggleNotifications({dispatch})}
+export const OtherPane = ({dispatch, state}) => {
+  const onToggleGlobalNotifications = event => {
+    event.stopPropagation();
+    toggleNotifications({dispatch})();
+  };
+  return isPaneActive(state)(OtherPane.id) && html`
+    <h2 class='title'>Other</h2>
+    <${Card} className='settings__other'>
+      <div style=${{display: 'flex', justifyContent: 'flex-start' /* eslint-disable no-warning-comments *//*
+      TODO: remove when Bulma is out
+      */}}>
+        <${HorizontalField} label='Theme' data-testid='settings-theme-select'>
+          <${Select} value=${state.theme} onChange=${e => setTheme({dispatch})(e.target.value)}>
+            <${Select.Option} value='system'>system</${Select.Option}>
+            <${Select.Option} value='light'>light</${Select.Option}>
+            <${Select.Option} value='dark'>dark</${Select.Option}>
+          </${Select}>
+        </${HorizontalField}>
+      </div>
+      <${Card.Divider} />
+      <${SettingsOption}
+          className='settings__global-notifications'
+          label='Disable notifications globally'
+          icon=${state.disableNotificationsGlobally ? Icon.notificationsOff : Icon.notifications}
+          checked=${state.disableNotificationsGlobally}
+          onClick=${onToggleGlobalNotifications}
       />
-    </div>
-    <${Card.Divider} />
-    <div class='is-italic' data-testid='settings-electronim-version'>
-      ElectronIM version ${ELECTRONIM_VERSION}
-    </div>
-  </${Card}>
-`;
+      <${Card.Divider} />
+      <div class='is-italic' data-testid='settings-electronim-version'>
+        ElectronIM version ${ELECTRONIM_VERSION}
+      </div>
+    </${Card}>
+  `;
+};
 
 OtherPane.id = 'other';
diff --git a/src/settings/settings.services.browser.mjs b/src/settings/settings.services.browser.mjs
index 68609e9..7f1a071 100644
--- a/src/settings/settings.services.browser.mjs
+++ b/src/settings/settings.services.browser.mjs
@@ -13,7 +13,8 @@
    See the License for the specific language governing permissions and
    limitations under the License.
  */
-import {html, Card, Icon, IconButton, Switch, TextField} from '../components/index.mjs';
+import {html, Card, Icon, IconButton, TextField} from '../components/index.mjs';
+import {SettingsOption} from './settings.common.browser.mjs';
 import {ACTIONS, addTab, isPaneActive, setTabProperty, toggleTabProperty} from './settings.reducer.browser.mjs';
 
 const disabledIcon = disabled => (disabled === true ? Icon.visibilityOff : Icon.visibility);
@@ -41,17 +42,14 @@ const TabAdvancedSettings = (
   };
   return html`
     <div class='settings__tab-advanced'>
-      <div
-        class='settings__option sandboxed-toggle' onClick=${onSandboxClick}
+      <${SettingsOption}
+        onClick=${onSandboxClick}
+        checked=${sandboxed}
+        icon=${sandboxedIcon(sandboxed)}
+        label='Sandbox'
+        className='sandboxed-toggle'
         title='Use an isolated/sandboxed session for this tab'
-      >
-        <${Icon}>${sandboxedIcon(sandboxed)}</${Icon}>
-        <span class='settings__option-label'>Sandbox</span>
-        <${Switch}
-          checked=${sandboxed} onClick=${onSandboxClick}
-          title='Use an isolated/sandboxed session for this tab'
-        />
-      </div>
+      />
     </div>
   `;
 };


Test patch:
diff --git a/src/settings/__tests__/settings.other.browser.test.mjs b/src/settings/__tests__/settings.other.browser.test.mjs
index 64c0f90..d5ca092 100644
--- a/src/settings/__tests__/settings.other.browser.test.mjs
+++ b/src/settings/__tests__/settings.other.browser.test.mjs
@@ -48,15 +48,27 @@ describe('Settings (Other) in Browser test suite', () => {
       }));
     }, 10000);
   });
-  test('Toggle global notifications should check input', async () => {
-    // Given
-    const $notificationCheckbox = document.querySelector('.settings__global-notifications input');
-    expect($notificationCheckbox.checked).toBe(false);
-    // When
-    fireEvent.click($notificationCheckbox);
-    // Then
-    await waitFor(() => {
-      expect($notificationCheckbox.checked).toBe(true);
+  describe('Toggle global notifications', () => {
+    let $notificationsSwitch;
+    beforeEach(() => {
+      $notificationsSwitch = document.querySelector('.settings__global-notifications .switch input');
+    });
+    test('when notifications enabled, should check input', async () => {
+      // Given
+      expect($notificationsSwitch.checked).toBe(false);
+      // When
+      fireEvent.click($notificationsSwitch);
+      // Then
+      await waitFor(() => expect($notificationsSwitch.checked).toBe(true));
+    });
+    test('when notifications disabled, should uncheck input', async () => {
+      // Given
+      fireEvent.click($notificationsSwitch);
+      await waitFor(() => expect($notificationsSwitch.checked).toBe(true));
+      // When
+      fireEvent.click($notificationsSwitch);
+      // Then
+      await waitFor(() => expect($notificationsSwitch.checked).toBe(false));
     });
   });
   test('ElectronIM version is visible', async () => {