Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

fix(core:button): disabled/loading state #6183

Merged
merged 1 commit into from
Jul 28, 2021
Merged

fix(core:button): disabled/loading state #6183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 3 additions & 15 deletions packages/core/src/button/base-button.element.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
cursor: pointer;
font-size: var(--font-size);
height: 100%;
justify-content: center;
line-height: 1em;
min-width: var(--min-width);
overflow: visible;
Expand All @@ -52,21 +51,10 @@
@include center-content('block');
height: 100%;
}
}

.button-status-icon {
cds-icon {
@include min-equilateral(#{$cds-global-space-8});
fill: var(--color);
}

cds-icon[shape='error-standard'] {
--color: #{$cds-alias-status-danger};
}

.button-spinner {
--ring-color: #{$cds-global-color-construction-400};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was able to use the component properties and simplify the CSS

}
cds-progress-circle {
--ring-color: #{$cds-global-color-construction-400};
}

:host(:active) .private-host,
Expand Down
119 changes: 58 additions & 61 deletions packages/core/src/button/button.element.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { html } from 'lit';
import { CdsButton, ClrLoadingState, iconSpinner } from '@cds/core/button';
import { CdsButton, ClrLoadingState } from '@cds/core/button';
import '@cds/core/badge/register.js';
import '@cds/core/button/register.js';
import { componentIsStable, createTestElement, getComponentSlotContent, removeTestElement } from '@cds/core/test';
Expand Down Expand Up @@ -221,15 +221,6 @@ describe('button element', () => {
});

describe('LoadingStateChange', () => {
it('should fallback to default state as expected', async () => {
await componentIsStable(component);
component.loadingState = null;
await componentIsStable(component);
expect(component.loadingState).toEqual(ClrLoadingState.default);
expect(component.hasAttribute('disabled')).toEqual(false);
expect(component.style.getPropertyValue('width')).toBe('');
});

it('should set default state as expected', async () => {
await componentIsStable(component);
component.loadingState = ClrLoadingState.default;
Expand Down Expand Up @@ -280,51 +271,74 @@ describe('button element', () => {
component.disabled = true;
component.loadingState = ClrLoadingState.default;
await componentIsStable(component);
expect(component.disabled).toBeTruthy();
});
});

describe('Button link', () => {
let testLinkElement: HTMLElement;
let anchor: HTMLAnchorElement;
let anchorButton: HTMLButtonElement;
component.loadingState = ClrLoadingState.loading;
await componentIsStable(component);
expect(component.disabled).toBeTruthy();

beforeEach(async () => {
testLinkElement = await createTestElement(html` <a href="about"><cds-button>About</cds-button></a> `);
component.loadingState = ClrLoadingState.success;
await componentIsStable(component);
expect(component.disabled).toBeTruthy();

anchor = testLinkElement.querySelector<HTMLAnchorElement>('a');
anchorButton = testLinkElement.querySelector<HTMLButtonElement>('cds-button');
component.loadingState = ClrLoadingState.default;
await componentIsStable(component);
expect(component.disabled).toBeTruthy();
});

afterEach(() => {
removeTestElement(testLinkElement);
it('should default to spinner size to "18"', async () => {
component.loadingState = ClrLoadingState.loading;
await componentIsStable(component);
expect(component.shadowRoot.querySelector<any>('cds-progress-circle').size).toBe('18');
});

it('should render a link properly', async () => {
await componentIsStable(anchorButton);
expect(anchor.style.lineHeight).toBe('0');
expect(anchor.style.textDecoration).toBe('none');
it('should set spinner size to "12" if button size "sm"', async () => {
component.loadingState = ClrLoadingState.loading;
component.size = 'sm';
await componentIsStable(component);
expect(component.shadowRoot.querySelector<any>('cds-progress-circle').size).toBe('12');
});
});
});

it('should set button to be readonly', async () => {
await componentIsStable(anchorButton);
expect(anchor.querySelector('cds-button').readonly).toBe(true);
});
describe('Button link', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this describe since it was nested in the describe above which made it a bit confusing what component it was referencing.

let testLinkElement: HTMLElement;
let anchor: HTMLAnchorElement;
let anchorButton: HTMLButtonElement;

it('should not trigger button click if link', async () => {
await componentIsStable(anchorButton);
const o = {
f: () => {
// Do nothing
},
};
spyOn(o, 'f');
anchorButton.addEventListener('click', o.f);
anchor.focus();
anchor.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' }));
beforeEach(async () => {
testLinkElement = await createTestElement(html` <a href="about"><cds-button>About</cds-button></a> `);
anchor = testLinkElement.querySelector<HTMLAnchorElement>('a');
anchorButton = testLinkElement.querySelector<HTMLButtonElement>('cds-button');
});

expect(o.f).not.toHaveBeenCalled();
});
afterEach(() => {
removeTestElement(testLinkElement);
});

it('should render a link properly', async () => {
await componentIsStable(anchorButton);
expect(anchor.style.lineHeight).toBe('0');
expect(anchor.style.textDecoration).toBe('none');
});

it('should set button to be readonly', async () => {
await componentIsStable(anchorButton);
expect(anchor.querySelector('cds-button').readonly).toBe(true);
});

it('should not trigger button click if link', async () => {
await componentIsStable(anchorButton);
const o = {
f: () => {
// Do nothing
},
};
spyOn(o, 'f');
anchorButton.addEventListener('click', o.f);
anchor.focus();
anchor.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' }));

expect(o.f).not.toHaveBeenCalled();
});
});

Expand All @@ -346,23 +360,6 @@ describe('buttonSlots: ', () => {
});
});

describe('iconSpinner(): ', () => {
it('should default to spinner size to "18"', () => {
const templateResult = iconSpinner('lg');
expect(templateResult.values.indexOf('18') > -1).toBe(true);
});

it('should set spinner size to "18" if not passed size "sm"', () => {
const templateResult = iconSpinner('anything-at-all');
expect(templateResult.values.indexOf('18') > -1).toBe(true);
});

it('should set spinner size to "12" if size "sm"', () => {
const templateResult = iconSpinner('sm');
expect(templateResult.values.indexOf('12') > -1).toBe(true);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the rest of the loading state specs

});

describe('button keyboard interaction: ', () => {
it('should add active attr on click', async () => {
const element = await createTestElement(html`<cds-button>Text slot</cds-button>`);
Expand Down
103 changes: 32 additions & 71 deletions packages/core/src/button/button.element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,11 @@
* The full license information can be found in LICENSE in the root directory of this project.
*/

import { baseStyles, CdsBaseButton, getElementWidth, property, spanWrapper } from '@cds/core/internal';
import { ClarityIcons } from '@cds/core/icon/icon.service.js';
import { errorStandardIcon } from '@cds/core/icon/shapes/error-standard.js';
import { checkIcon } from '@cds/core/icon/shapes/check.js';
import { baseStyles, CdsBaseButton, getElementWidth, property } from '@cds/core/internal';
import { html } from 'lit';
import { query } from 'lit/decorators/query.js';
import baseButtonStyles from './base-button.element.scss';
import styles from './button.element.scss';

export const iconSpinner = (size: string) => {
const spinnerSize = size === 'sm' ? '12' : '18';
return html`<span class="button-status-icon" cds-layout="horizontal align:center"><cds-progress-circle class="button-spinner" size="${spinnerSize}" status="info"></cds-progress-circle></span></span>`;
};

export const iconCheck = html`<span class="button-status-icon" cds-layout="horizontal align:center"
><cds-icon shape="check" status="success" cds-layout="align:center"></cds-icon
></span>`;

export const iconError = html`<span class="button-status-icon" cds-layout="horizontal align:center"
><cds-icon shape="error-standard" cds-layout="align:center"></cds-icon
></span>`;

export const enum ClrLoadingState {
default = 'default',
loading = 'loading',
Expand Down Expand Up @@ -89,8 +72,6 @@ export class CdsButton extends CdsBaseButton {
@property({ type: Boolean })
block = false;

@query('.private-host') privateHost: HTMLElement;

/**
* @type {default | loading | success | error}
* Changes the button content based on the value passed.
Expand All @@ -100,75 +81,55 @@ export class CdsButton extends CdsBaseButton {
* - `success`: disables the button and shows a check mark inside the button; auto-triggers to change back to DEFAULT state after 1000 ms
* - `error`: shows the content of the button (in the context of application, this state is usually entered from a LOADING state. the application should show appropriate error message)
*/
@property({ type: String })
loadingState: keyof typeof ClrLoadingState = ClrLoadingState.default;

constructor() {
super();
ClarityIcons.addIcons(errorStandardIcon, checkIcon);
@property({ type: String }) get loadingState() {
return this._loadingState;
}

firstUpdated(props: Map<string, any>) {
super.firstUpdated(props);

// Find and wrap any text nodes into span elements
spanWrapper(this.childNodes);

if (this.loadingState !== ClrLoadingState.default) {
this.updateLoadingState();
set loadingState(value: keyof typeof ClrLoadingState) {
// track prior disabled state to set prior value after button is re-enabled from a loading state
if (this._loadingState === ClrLoadingState.default) {
this._disabled = this.disabled;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _disabled property was the key fix. To preserve the disabled state while not breaking or causing side effects we need to grab what the current disabled state is when updating from default to something else. This way when we go back to default we can use what the prior disabled state was.

}
}

update(props: Map<string, any>) {
if (this.privateHost && props.has('loadingState')) {
this.updateLoadingState();
if (value === ClrLoadingState.default) {
this.enableButton();
} else {
this.disableButton();
}
super.update(props);

const oldValue = this._loadingState;
this._loadingState = value;
this.requestUpdate('loadingState', oldValue);
}

private _loadingState: keyof typeof ClrLoadingState = ClrLoadingState.default;
private _disabled = false;

render() {
const loadingState = this.loadingState;
return html`<div class="private-host">
<div cds-layout="horizontal gap:sm wrap:none align:center">
${loadingState === ClrLoadingState.success ? iconCheck : ''}
${loadingState === ClrLoadingState.error ? iconError : ''}
${loadingState === ClrLoadingState.loading ? iconSpinner(this.size) : ''}
${loadingState === ClrLoadingState.default
? html`<slot @slotchange=${() => spanWrapper(this.childNodes)}></slot>`
: ''}
</div>
return html`<div class="private-host" cds-layout="horizontal gap:sm wrap:none align:center">
${this.loadingState === ClrLoadingState.success
? html`<cds-icon shape="check" status="success" size="18"></cds-icon>`
: ''}
${this.loadingState === ClrLoadingState.error
? html`<cds-icon shape="error-standard" status="danger" size="18"></cds-icon>`
: ''}
${this.loadingState === ClrLoadingState.loading
? html`<cds-progress-circle .size=${this.size === 'sm' ? '12' : '18'} status="info"></cds-progress-circle>`
: ''}
${this.loadingState === ClrLoadingState.default ? html`<slot></slot>` : ''}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Layout was simplified and didn't need the wrapper elements anymore

</div>`;
}

static styles = [baseStyles, baseButtonStyles, styles];

private updateLoadingState() {
if (this.disabled) {
return;
}
switch (this.loadingState) {
case ClrLoadingState.loading:
this.disableButton();
return;
case ClrLoadingState.success:
this.disableButton();
return;
case ClrLoadingState.error:
this.disableButton();
return;
default:
this.enableButton();
}
}

private disableButton() {
private async disableButton() {
await this.updateComplete;
this.style.width = getElementWidth(this);
this.disabled = true;
}

private enableButton() {
this.loadingState = ClrLoadingState.default;
this.style.removeProperty('width');
this.disabled = false;
this.disabled = this._disabled;
}
}
13 changes: 1 addition & 12 deletions packages/core/src/button/icon-button.element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@
*/

import { baseStyles, property } from '@cds/core/internal';
import { html } from 'lit';
import baseButtonStyles from './base-button.element.scss';
import styles from './icon-button.element.scss';
import { CdsButton, ClrLoadingState, iconCheck, iconSpinner } from './button.element.js';
import { CdsButton } from './button.element.js';

/**
* Icon buttons give applications a compact alternative to communicate action and direct user intent.
Expand Down Expand Up @@ -44,15 +43,5 @@ export class CdsIconButton extends CdsButton {
@property({ type: String, required: 'warning' })
ariaLabel: string;

render() {
return html`
<div class="private-host">
${this.loadingState === ClrLoadingState.loading ? iconSpinner(this.size) : ''}
${this.loadingState === ClrLoadingState.success ? iconCheck : ''}
${this.loadingState === ClrLoadingState.default ? html`<slot></slot>` : ''}
</div>
`;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Icon button was the same template/logic duplicated as the inherited template

static styles = [baseStyles, baseButtonStyles, styles];
}
5 changes: 5 additions & 0 deletions packages/core/src/button/register.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import { registerElementSafely } from '@cds/core/internal';
import { CdsButton } from './button.element.js';
import { CdsIconButton } from './icon-button.element.js';
import { CdsInlineButton } from './inline-button.element.js';
import { ClarityIcons } from '@cds/core/icon/icon.service.js';
import { errorStandardIcon } from '@cds/core/icon/shapes/error-standard.js';
import { checkIcon } from '@cds/core/icon/shapes/check.js';

ClarityIcons.addIcons(errorStandardIcon, checkIcon);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the side effects to the register to follow the rest of the component register files


registerElementSafely('cds-button', CdsButton);
registerElementSafely('cds-icon-button', CdsIconButton);
Expand Down