Remove magin on buttons in modal header and footer

This is to avoid magic numbers for paddings in code and tests as the margin would have to be subtracted from the paddings in header and footer for the design to look correct.
kirbydesign · Apr 25, 2023 · d5fcbae · d5fcbae
1 parent 664c734
commit d5fcbae
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 27 deletions.
diff --git a/libs/designsystem/button/src/button.component.scss b/libs/designsystem/button/src/button.component.scss
@@ -361,6 +361,23 @@ $button-width: (
   @include button-no-decoration;
 }
 
+:host-context(.kirby-modal ion-header ion-toolbar ion-buttons) {
+  margin: 0;
+}
+
+:host-context(kirby-modal-footer) {
+  margin-top: 0;
+  margin-bottom: 0;
+
+  &:first-child {
+    margin-left: 0;
+  }
+
+  &:last-child {
+    margin-right: 0;
+  }
+}
+
 /*
 * TEMPORARY MORE-MENU
 * Button content should not be space-between when only an icon is slotted 

diff --git a/libs/designsystem/modal/src/modal-wrapper/modal-wrapper.component.scss b/libs/designsystem/modal/src/modal-wrapper/modal-wrapper.component.scss
@@ -2,6 +2,8 @@
 
 // Global modal styling can be found at scss/base/_ionic.scss
 
+$toolbar-bottom-border-width: 1px;
+
 @mixin contain-content() {
   padding-top: 0;
   position: relative;
@@ -36,36 +38,36 @@
 
 @mixin toolbar-phablet-or-bigger() {
   @include utils.media('>=medium') {
-    $toolbar-padding: 20px;
-
-    --padding-start: #{$toolbar-padding};
-    --padding-end: #{$toolbar-padding};
-    --padding-bottom: #{$toolbar-padding};
-    --padding-top: #{$toolbar-padding};
+    --padding-start: #{utils.size('m')};
+    --padding-end: #{utils.size('m')};
+    --padding-bottom: calc(#{utils.size('m')} - #{$toolbar-bottom-border-width});
+    --padding-top: #{utils.size('m')};
 
     padding-inline: 0;
   }
 }
 
 ion-header {
   box-sizing: border-box;
-  border-bottom: 1px solid var(--kirby-modal-background, utils.get-color('medium'));
-  transition: border-color 100ms linear;
 
   ion-toolbar {
     @include toolbar-phablet-or-bigger;
 
-    --padding-start: #{utils.size('xxxs')};
-    --padding-end: #{utils.size('xxxs')};
-    --padding-bottom: #{utils.size('xxxs')};
-    --padding-top: #{utils.size('xxxs')};
+    --padding-start: #{utils.size('xxs')};
+    --padding-end: #{utils.size('xxs')};
+    --padding-bottom: calc(#{utils.size('xxs')} - #{$toolbar-bottom-border-width});
+    --padding-top: #{utils.size('xxs')};
     --border-width: 0;
     --background: transparent;
     --color: var(--kirby-modal-color, #{utils.get-color('black')});
 
     button {
       color: var(--color);
     }
+
+    border-bottom: $toolbar-bottom-border-width solid
+      var(--kirby-modal-background, utils.get-color('medium'));
+    transition: border-color 100ms linear;
   }
 }
 
@@ -85,8 +87,9 @@ ion-header {
 
 :host(:not(.content-scrolled)) {
   @include utils.media('<medium') {
-    ion-header {
-      border-bottom: 1px solid var(--kirby-modal-background, utils.get-color('background-color'));
+    ion-header ion-toolbar {
+      border-bottom: $toolbar-bottom-border-width solid
+        var(--kirby-modal-background, utils.get-color('background-color'));
       transition: border-color 100ms linear;
     }
   }
@@ -154,9 +157,6 @@ ion-content {
     ion-header {
       --padding-top: 0px;
 
-      // no border for header element when collapsible title is in the content area
-      border: none;
-
       ion-toolbar:first-of-type {
         @include utils.media('>=medium') {
           --padding-start: #{utils.size('s')};
@@ -167,6 +167,9 @@ ion-content {
         --padding-bottom: #{utils.size('l')};
         --padding-start: #{utils.size('xxs')};
         --padding-end: #{utils.size('xxs')};
+
+        // no border for header element when collapsible title is in the content area
+        border: none;
       }
     }
   }

diff --git a/libs/designsystem/modal/src/modal/footer/modal-footer.component.scss b/libs/designsystem/modal/src/modal/footer/modal-footer.component.scss
@@ -10,11 +10,11 @@ ion-footer {
   align-items: center;
   background-color: var(--kirby-modal-footer-background, utils.get-color('white'));
   color: var(--kirby-modal-footer-color, utils.get-color('white-contrast'));
-  padding: utils.size('xs');
+  padding: utils.size('s');
   padding-bottom: calc(#{utils.size('xs')} + var(--kirby-modal-footer-safe-area-bottom, 0px));
 
   @include utils.media('>=medium') {
-    padding: 20px;
+    padding: utils.size('m');
   }
 }
 

diff --git a/libs/designsystem/modal/src/modal/footer/modal-footer.component.spec.ts b/libs/designsystem/modal/src/modal/footer/modal-footer.component.spec.ts
@@ -7,10 +7,10 @@ import { TestHelper } from '@kirbydesign/designsystem/testing';
 import { ThemeColorDirective } from '@kirbydesign/designsystem/shared';
 import { ModalFooterComponent } from '../footer/modal-footer.component';
 
-const getColor = DesignTokenHelper.getColor;
+const { getColor, size } = DesignTokenHelper;
 
 const KEYBOARD_HEIGHT = 216; // sample value, depends upon device
-const BASE_PADDING_PX = '20px';
+const BASE_PADDING_PX = size('m');
 const BASE_PADDING_SMALL_SCREEN = 12;
 const BASE_PADDING_SMALL_SCREEN_PX = `${BASE_PADDING_SMALL_SCREEN}px`;
 const SAFE_AREA_BOTTOM = 22;
@@ -63,15 +63,15 @@ describe('ModalFooterComponent', () => {
     });
 
     it('when --kirby-safe-area-bottom is not set', () => {
-      expect(ionFooterElement).toHaveComputedStyle({ 'padding-bottom': '20px' });
+      expect(ionFooterElement).toHaveComputedStyle({ 'padding-bottom': size('m') });
     });
 
     /**
      * Temporaly removed, see #2736
      */
     xit('when --kirby-safe-area-bottom is set', () => {
       setSafeAreaBottom();
-      expect(ionFooterElement).toHaveComputedStyle({ 'padding-bottom': '20px' });
+      expect(ionFooterElement).toHaveComputedStyle({ 'padding-bottom': size('m') });
     });
 
     describe('on small screens', () => {

diff --git a/libs/designsystem/modal/src/modal/services/modal.helper.spec.ts b/libs/designsystem/modal/src/modal/services/modal.helper.spec.ts
@@ -362,11 +362,14 @@ describe('ModalHelper', () => {
       it('should have correct padding on tablet/desktop', () => {
         const toolbarContainer = ionToolbarElement.shadowRoot.querySelector('.toolbar-container');
 
-        // visually toolbar is 24px padding, but 4px comes from toolbar button margins
-        const toolbarPadding = '20px';
+        //subtract border thickness from expected bottom padding
+        const expectedToolbarContainerPaddingBottom = parseInt(size('m')) - 1 + 'px';
 
         expect(toolbarContainer).toHaveComputedStyle({
-          padding: toolbarPadding,
+          'padding-top': size('m'),
+          'padding-right': size('m'),
+          'padding-bottom': expectedToolbarContainerPaddingBottom,
+          'padding-left': size('m'),
         });
         expect(ionToolbarElement).toHaveComputedStyle({
           'padding-top': '0px',
@@ -377,8 +380,14 @@ describe('ModalHelper', () => {
         await TestHelper.resizeTestWindow(TestHelper.screensize.phone);
         const toolbarContainer = ionToolbarElement.shadowRoot.querySelector('.toolbar-container');
 
+        //subtract border thickness from expected bottom padding
+        const expectedToolbarContainerPaddingBottom = parseInt(size('xxs')) - 1 + 'px';
+
         expect(toolbarContainer).toHaveComputedStyle({
-          padding: size('xxxs'),
+          'padding-top': size('xxs'),
+          'padding-right': size('xxs'),
+          'padding-bottom': expectedToolbarContainerPaddingBottom,
+          'padding-left': size('xxs'),
         });
         expect(ionToolbarElement).toHaveComputedStyle({
           'padding-top': '0px',