fix(pie-checkbox): DSW-000 design review comments (#1660)

justeattakeaway · Aug 2, 2024 · bde5fdf · bde5fdf
1 parent a0c92b8
commit bde5fdf
Show file tree

Hide file tree

Showing 9 changed files with 149 additions and 83 deletions.
diff --git a/.changeset/blue-keys-shout.md b/.changeset/blue-keys-shout.md
@@ -0,0 +1,6 @@
+---
+"@justeattakeaway/pie-checkbox": minor
+---
+
+[Changed] - cursor styling for disabled state changed from default to not-allowed.
+[Added] - transition styles to border colour in addition to background colour.
diff --git a/.changeset/wild-scissors-bathe.md b/.changeset/wild-scissors-bathe.md
@@ -0,0 +1,7 @@
+---
+"@justeattakeaway/pie-checkbox-group": minor
+---
+
+[Changed] - replaced label prop with a slot.
+[Changed] - increased spacing after label and before assistive text.
+[Changed] - reduced spacing detween checkboxes in inline mode.
diff --git a/apps/pie-storybook/stories/pie-checkbox-group.stories.ts b/apps/pie-storybook/stories/pie-checkbox-group.stories.ts
@@ -1,22 +1,31 @@
-import { html } from 'lit';
+import { html, nothing } from 'lit';
 import { ifDefined } from 'lit/directives/if-defined.js';
 /* eslint-disable import/no-duplicates */
 import '@justeattakeaway/pie-checkbox-group';
-import { CheckboxGroupProps, defaultProps, statusTypes } from '@justeattakeaway/pie-checkbox-group';
+import { CheckboxGroupProps as CheckboxGroupBase, defaultProps, statusTypes } from '@justeattakeaway/pie-checkbox-group';
 /* eslint-enable import/no-duplicates */
 
 import { type StoryMeta } from '../types';
 import { createStory } from '../utilities';
 
 import '@justeattakeaway/pie-link';
 import '@justeattakeaway/pie-checkbox';
+import '@justeattakeaway/pie-form-label';
+
+// Extending the props type definition to include storybook specific properties for controls
+type CheckboxGroupProps = CheckboxGroupBase & {
+    labelSlot: typeof slotOptions[number];
+};
 
 type CheckboxGroupStoryMeta = StoryMeta<CheckboxGroupProps>;
 
 const defaultArgs: CheckboxGroupProps = {
     ...defaultProps,
+    labelSlot: 'None',
 };
 
+const slotOptions = ['Checkbox Group Label', 'None'] as const;
+
 const checkboxGroupStoryMeta: CheckboxGroupStoryMeta = {
     title: 'Checkbox Group',
     component: 'pie-checkbox-group',
@@ -25,9 +34,11 @@ const checkboxGroupStoryMeta: CheckboxGroupStoryMeta = {
             description: 'The name associated with the group.',
             control: 'text',
         },
-        label: {
-            description: 'The visible label for the checkbox group.',
-            control: 'text',
+        labelSlot: {
+            name: 'Label Slot',
+            description: '<b>**Not a component Prop</b><br><br>Use the `label` slot to pass <pie-form-label> component with all the relevant props.',
+            control: 'select',
+            options: slotOptions,
         },
         isInline: {
             description: 'Inline (horizontal) positioning of checkbox items.',
@@ -72,32 +83,41 @@ export default checkboxGroupStoryMeta;
 
 const Template = ({
     name,
-    label,
     isInline,
     assistiveText,
     status,
     disabled,
-}: CheckboxGroupProps) => html`
-    <div style="max-width: 400px">
-        <p>Please note, the checkboxes are separate components. See
-        <pie-link href="/?path=/story/checkbox--default">pie-checkbox</pie-link>.</p>
-        <pie-checkbox-group
-            name="${ifDefined(name)}"
-            label="${ifDefined(label)}"
-            assistiveText="${ifDefined(assistiveText)}"
-            ?isInline=${isInline}
-            status=${ifDefined(status)}
-            ?disabled="${disabled}"
-        >
-            <pie-checkbox name="checkbox-one">checkbox 1</pie-checkbox>
-            <pie-checkbox name="checkbox-two">checkbox 2</pie-checkbox>
-            <pie-checkbox name="checkbox-three">checkbox 3 longer label</pie-checkbox>
-            <pie-checkbox name="checkbox-three">checkbox 4</pie-checkbox>
-            <pie-checkbox name="checkbox-three">checkbox 5 even longer label: Lorem ipsum dolor sit amet,
-            consectetur adipiscing elit.</pie-checkbox>
-            <pie-checkbox name="checkbox-three">checkbox 6</pie-checkbox>
-        </pie-checkbox-group>
-    </div>
-`;
+    labelSlot,
+}: CheckboxGroupProps) => {
+    function renderLabelSlot (slotValue: typeof slotOptions[number]) {
+        if (slotValue === slotOptions[0]) {
+            return html`<pie-form-label slot="label">Checkbox Group Label</pie-form-label>`;
+        }
+        return nothing;
+    }
+
+    return html`
+        <div style="max-width: 400px">
+            <p>Please note, the checkboxes are separate components. See
+            <pie-link href="/?path=/story/checkbox--default">pie-checkbox</pie-link>.</p>
+            <pie-checkbox-group
+                name="${ifDefined(name)}"
+                assistiveText="${ifDefined(assistiveText)}"
+                ?isInline=${isInline}
+                status=${ifDefined(status)}
+                ?disabled="${disabled}"
+            >
+                ${renderLabelSlot(labelSlot)}
+                <pie-checkbox name="checkbox-one">checkbox 1</pie-checkbox>
+                <pie-checkbox name="checkbox-two">checkbox 2</pie-checkbox>
+                <pie-checkbox name="checkbox-three">checkbox 3 longer label</pie-checkbox>
+                <pie-checkbox name="checkbox-three">checkbox 4</pie-checkbox>
+                <pie-checkbox name="checkbox-three">checkbox 5 even longer label: Lorem ipsum dolor sit amet,
+                consectetur adipiscing elit.</pie-checkbox>
+                <pie-checkbox name="checkbox-three">checkbox 6</pie-checkbox>
+            </pie-checkbox-group>
+        </div>
+    `;
+};
 
 export const Default = createStory<CheckboxGroupProps>(Template, defaultArgs)();
diff --git a/packages/components/pie-checkbox-group/README.md b/packages/components/pie-checkbox-group/README.md
@@ -76,12 +76,10 @@ import { PieCheckboxGroup } from '@justeattakeaway/pie-checkbox-group/dist/react
 | Property | Type | Default | Description |
 |---|---|---|---|
 | `name` | string | - | The name associated with the group. |
-| `label` | string | - | The label value of the component |
 | `disabled` | boolean | - | Same as the HTML disabled attribute - indicates whether or not the checkbox group is disabled. |
 | `assistiveText` | `string` | - | Allows assistive text to be displayed below the checkbox group. |
 | `status` | `'default'`, `'error'`, `'success'` | `'default'` | The status of the checkbox group / assistive text. If you use `status` you must provide an `assistiveText` prop value for accessibility purposes. |
 
-
 In your markup or JSX, you can then use these to set the properties for the `pie-checkbox-group` component:
 
 ```html
@@ -100,6 +98,13 @@ In your markup or JSX, you can then use these to set the properties for the `pie
 </PieCheckboxGroup>
 ```
 
+## Slots
+
+| Slot | Description |
+|---|---|
+| `default` | Pass PieCheckbox components as direct children for the CheckboxGroup. |
+| `label` | Pass PieFormLabel to render the checkbox group label. |
+
 ## Events
 | Event | Type | Description |
 |-------|------|-------------|

diff --git a/packages/components/pie-checkbox-group/src/checkbox-group.scss b/packages/components/pie-checkbox-group/src/checkbox-group.scss
@@ -14,9 +14,14 @@
         display: flex;
     }
 
+    ::slotted(pie-form-label) {
+        display: block;
+        padding-block-end: var(--checkbox-group-label-offset);
+    }
+
     &:not(.c-checkboxGroup--inline) {
-        ::slotted(pie-checkbox:not(:first-child)) {
-            margin-block-start: var(--checkbox-group-offset);
+        ::slotted(pie-checkbox:not(:last-child)) {
+            margin-block-end: var(--checkbox-group-offset);
         }
     }
 }
@@ -28,26 +33,22 @@
     flex-wrap: wrap;
 }
 
-.c-checkboxGroup-label {
-    max-width: 100%;
-    white-space: normal;
-    padding: 0;
-    font-size: p.font-size(--dt-font-body-strong-s-size);
-    line-height: p.line-height(--dt-font-body-strong-s-line-height);
-    font-weight: var(--dt-font-body-strong-s-weight);
-    color: var(--dt-color-content-default);
-
-    margin-block-end: var(--checkbox-group-label-offset);
-}
-
 .c-checkboxGroup.c-checkboxGroup--inline {
-    margin: calc(-1 * var(--checkbox-group-offset--inline) / 2) calc(-1 * var(--checkbox-group-offset--inline) / 2);
+    margin: calc(-1 * var(--checkbox-group-offset) / 2) calc(-1 * var(--checkbox-group-offset--inline) / 2);
 
     ::slotted(pie-checkbox) {
-        margin: calc(var(--checkbox-group-offset--inline) / 2) calc(var(--checkbox-group-offset--inline) / 2);
+        margin: calc(var(--checkbox-group-offset) / 2) calc(var(--checkbox-group-offset--inline) / 2);
     }
 
-    .c-checkboxGroup-label {
-        margin: 0 calc(var(--checkbox-group-offset--inline) / 2) calc(-1 * (var(--checkbox-group-offset--inline) / 2 - var(--checkbox-group-label-offset)));
+    ::slotted(pie-form-label) {
+        margin: 0 calc(var(--checkbox-group-offset--inline) / 2) calc(-1 * (var(--checkbox-group-offset) / 2 - var(--checkbox-group-label-offset)));
+        padding-block-end: 0;
     }
 }
+
+.c-checkboxGroup-assistiveText {
+    --checkbox-group-assistive-text-offset: var(--dt-spacing-a);
+
+    display: block;
+    margin-block-start: var(--checkbox-group-assistive-text-offset);
+}
diff --git a/packages/components/pie-checkbox-group/src/defs.ts b/packages/components/pie-checkbox-group/src/defs.ts
@@ -8,11 +8,6 @@ export interface CheckboxGroupProps {
     */
     name?: string;
 
-    /**
-     * The label value of the component
-     */
-    label?: string;
-
     /**
      * Inline (horizontal) positioning of checkbox items
      */

diff --git a/packages/components/pie-checkbox-group/src/index.ts b/packages/components/pie-checkbox-group/src/index.ts
@@ -1,7 +1,7 @@
 import {
-    LitElement, html, unsafeCSS, PropertyValues,
+    LitElement, html, unsafeCSS, PropertyValues, nothing, type TemplateResult,
 } from 'lit';
-import { property, queryAssignedElements } from 'lit/decorators.js';
+import { property, queryAssignedElements, state } from 'lit/decorators.js';
 import {
     RtlMixin,
     defineCustomElement,
@@ -29,15 +29,16 @@ const assistiveTextId = 'assistive-text';
 /**
  * @tagname pie-checkbox-group
  * @slot - Default slot
+ * @slot label - The label slot
  * @event {CustomEvent} pie-checkbox-group-disabled - triggered after the disabled state of the checkbox group changes.
  * @event {CustomEvent} pie-checkbox-group-error - triggered after the state of the checkbox group changes to error.
  */
 export class PieCheckboxGroup extends FormControlMixin(RtlMixin(LitElement)) implements CheckboxGroupProps {
-    @property({ type: String })
-    public name?: CheckboxGroupProps['name'];
+    @state()
+        hasLabel = false;
 
     @property({ type: String })
-    public label?: CheckboxGroupProps['label'];
+    public name?: CheckboxGroupProps['name'];
 
     @property({ type: String })
     public assistiveText?: CheckboxGroupProps['assistiveText'];
@@ -54,6 +55,8 @@ export class PieCheckboxGroup extends FormControlMixin(RtlMixin(LitElement)) imp
 
     @queryAssignedElements({ selector: 'pie-checkbox' }) _slottedChildren: Array<HTMLElement> | undefined;
 
+    @queryAssignedElements({ slot: 'label' }) _labelSlot!: Array<HTMLElement>;
+
     private _handleDisabled () : void {
         this._slottedChildren?.forEach((child) => child.dispatchEvent(new CustomEvent(ON_CHECKBOX_GROUP_DISABLED, {
             bubbles: false, composed: false, detail: { disabled: this.disabled },
@@ -71,6 +74,25 @@ export class PieCheckboxGroup extends FormControlMixin(RtlMixin(LitElement)) imp
         });
     }
 
+    /**
+     * Function that updates the local `hasLabel` state in case
+     * when the label slot receives content.
+     * @private
+     */
+    private handleSlotChange (e: { target: HTMLSlotElement; }) {
+        const childNodes = e.target.assignedNodes({ flatten: true });
+        this.hasLabel = childNodes.length > 0;
+    }
+
+    /**
+     * Template for the label slot to correctly wrap it into a legend element when it has content.
+     * Called within the main render function.
+     * @private
+     */
+    private renderWrappedLabel (): TemplateResult | typeof nothing {
+        return this.hasLabel ? html`<legend><slot name='label' @slotchange=${this.handleSlotChange}></slot></legend>` : html`<slot name='label' @slotchange=${this.handleSlotChange}></slot>`;
+    }
+
     protected updated (_changedProperties: PropertyValues<this>): void {
         if (_changedProperties.has('disabled')) {
             this._handleDisabled();
@@ -84,7 +106,6 @@ export class PieCheckboxGroup extends FormControlMixin(RtlMixin(LitElement)) imp
     render () {
         const {
             name,
-            label,
             isInline,
             assistiveText,
             status,
@@ -104,13 +125,14 @@ export class PieCheckboxGroup extends FormControlMixin(RtlMixin(LitElement)) imp
                 data-test-id="pie-checkbox-group"
                 class="${classMap(classes)}"
             >
-                ${label && html`<legend class="c-checkboxGroup-label">${label}</legend>`}
+                ${this.renderWrappedLabel()}
                 <slot></slot>
             </fieldset>
             ${assistiveText && html`
                 <pie-assistive-text
                     id="${assistiveTextId}"
                     variant=${status}
+                    class="c-checkboxGroup-assistiveText"
                     data-test-id="pie-checkbox-group-assistive-text">
                         ${assistiveText}
                 </pie-assistive-text>`}