refactor(button): several improvements (#1019)
- remove click-area --> move styles to host::before - reduce css so that extending styles makes sense. Merge .btn with host - reduce the template and remove the if else construction inside the template - hide focus styles if they're not needed, for example, when an element receives focus via the mouse. - improve __clickDelegationHandler. Use current slotted native button instead of create new one - fix vertical align when 2 buttons are inline and one has icon. Example included. - add min-with to ensure target size for mobile, not only height - add test to check event.target in all situations (inside and outside form)
This commit is contained in:
Jorge del Casar
GitHub
parent
2dc85b14d3
commit
26b605939f
4 changed files with 151 additions and 75 deletions
12
.changeset/two-rats-tease.md
Normal file
12
.changeset/two-rats-tease.md
Normal file
|
|
@ -0,0 +1,12 @@
|
|||
---
|
||||
"@lion/button": minor
|
||||
---
|
||||
|
||||
Several button improvements
|
||||
|
||||
- remove click-area --> move styles to host::before
|
||||
- reduce css so that extending styles makes sense. Merge .btn with host.
|
||||
- reduce the template and remove the if else construction inside the template.
|
||||
- hide focus styles if they're not needed, for example, when an element receives focus via the mouse.
|
||||
- improve __clickDelegationHandler. Use current slotted native button instead of create new one.
|
||||
- fix vertical alignment when 2 buttons are inline and one has icon. Example included.
|
||||
|
|
@ -16,7 +16,7 @@ export default {
|
|||
```
|
||||
|
||||
```js preview-story
|
||||
export const main = () => html` <lion-button>Default</lion-button> `;
|
||||
export const main = () => html`<lion-button>Default</lion-button>`;
|
||||
```
|
||||
|
||||
## Features
|
||||
|
|
@ -60,20 +60,43 @@ export const handler = () => html`
|
|||
### Icon button
|
||||
|
||||
```js preview-story
|
||||
export const iconButton = () => html` <lion-button>${iconSvg(html)} Bug</lion-button> `;
|
||||
export const iconButton = () => html`<lion-button>${iconSvg(html)}Bug</lion-button>`;
|
||||
```
|
||||
|
||||
### Icon only button
|
||||
|
||||
```js preview-story
|
||||
export const iconOnly = () =>
|
||||
html` <lion-button aria-label="Bug"> ${iconSvg(html)} </lion-button> `;
|
||||
export const iconOnly = () => html`<lion-button aria-label="Bug">${iconSvg(html)}</lion-button>`;
|
||||
```
|
||||
|
||||
### Disabled button
|
||||
|
||||
```js preview-story
|
||||
export const disabled = () => html` <lion-button disabled>Disabled</lion-button> `;
|
||||
export const disabled = () => html`<lion-button disabled>Disabled</lion-button>`;
|
||||
```
|
||||
|
||||
### Multiple buttons inline
|
||||
|
||||
```js preview-story
|
||||
export const mainAndIconButton = () => html`
|
||||
<lion-button>Default</lion-button>
|
||||
<lion-button>${iconSvg(html)} Bug</lion-button>
|
||||
`;
|
||||
```
|
||||
|
||||
### Small button (minimum click area showed)
|
||||
|
||||
```js preview-story
|
||||
export const smallButton = () => html` <style>
|
||||
.small {
|
||||
padding: 4px;
|
||||
line-height: 1em;
|
||||
}
|
||||
.small::before {
|
||||
border: 1px dashed #000;
|
||||
}
|
||||
</style>
|
||||
<lion-button class="small">xs</lion-button>`;
|
||||
```
|
||||
|
||||
### Usage with native form
|
||||
|
|
@ -90,14 +113,14 @@ export const withinForm = () => html`
|
|||
<form
|
||||
@submit=${ev => {
|
||||
ev.preventDefault();
|
||||
console.log('submit handler');
|
||||
console.log('submit handler', ev.target);
|
||||
}}
|
||||
>
|
||||
<label for="firstNameId">First name</label>
|
||||
<input id="firstNameId" name="firstName" />
|
||||
<label for="lastNameId">Last name</label>
|
||||
<input id="lastNameId" name="lastName" />
|
||||
<lion-button @click=${() => console.log('click handler')}>Submit</lion-button>
|
||||
<lion-button @click=${ev => console.log('click handler', ev.target)}>Submit</lion-button>
|
||||
</form>
|
||||
`;
|
||||
```
|
||||
|
|
|
|||
|
|
@ -8,16 +8,6 @@ import {
|
|||
} from '@lion/core';
|
||||
import '@lion/core/src/differentKeyEventNamesShimIE.js';
|
||||
|
||||
// TODO: several improvements:
|
||||
// [1] remove click-area
|
||||
// [2] remove the native _button slot. We can detect and submit parent form without the slot.
|
||||
// [3] reduce css so that extending styles makes sense. Merge .btn with host
|
||||
// [4] reduce the template and remove the if else construction inside the template (an extra
|
||||
// div by default to support IE is fine) => <div id="${this._buttonId}"><slot></slot></div>
|
||||
// should be all needed
|
||||
// [5] do we need the before and after templates? Could be added by subclasser
|
||||
// [6] extract all functionality (except for form submission) into LionButtonMixin
|
||||
|
||||
const isKeyboardClickEvent = (/** @type {KeyboardEvent} */ e) => e.key === ' ' || e.key === 'Enter';
|
||||
const isSpaceKeyboardClickEvent = (/** @type {KeyboardEvent} */ e) => e.key === ' ';
|
||||
|
||||
|
|
@ -41,15 +31,10 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
|
|||
|
||||
render() {
|
||||
return html`
|
||||
<div class="btn">
|
||||
<div class="click-area"></div>
|
||||
${this._beforeTemplate()}
|
||||
${browserDetection.isIE11
|
||||
? html`<div id="${this._buttonId}"><slot></slot></div>`
|
||||
: html`<slot></slot>`}
|
||||
<div class="button-content" id="${this._buttonId}"><slot></slot></div>
|
||||
${this._afterTemplate()}
|
||||
<slot name="_button"></slot>
|
||||
</div>
|
||||
`;
|
||||
}
|
||||
|
||||
|
|
@ -67,24 +52,39 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
|
|||
return [
|
||||
css`
|
||||
:host {
|
||||
position: relative;
|
||||
display: inline-block;
|
||||
min-height: 40px; /* src = https://www.smashingmagazine.com/2012/02/finger-friendly-design-ideal-mobile-touchscreen-target-sizes/ */
|
||||
outline: 0;
|
||||
background-color: transparent;
|
||||
box-sizing: border-box;
|
||||
vertical-align: middle;
|
||||
line-height: 24px;
|
||||
background: #eee; /* minimal styling to make it recognizable as btn */
|
||||
padding: 8px; /* padding to fix with min-height */
|
||||
outline: none; /* focus style handled below */
|
||||
cursor: default; /* /* we should always see the default arrow, never a caret */
|
||||
}
|
||||
|
||||
.btn {
|
||||
min-height: 24px;
|
||||
:host::before {
|
||||
content: '';
|
||||
|
||||
/* center vertically and horizontally */
|
||||
position: absolute;
|
||||
top: 50%;
|
||||
left: 50%;
|
||||
transform: translate(-50%, -50%);
|
||||
|
||||
/* touch area (comes into play when button height goes below this one) */
|
||||
/* src = https://www.smashingmagazine.com/2012/02/finger-friendly-design-ideal-mobile-touchscreen-target-sizes/ */
|
||||
min-height: 40px;
|
||||
min-width: 40px;
|
||||
}
|
||||
|
||||
.button-content {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
position: relative;
|
||||
background: #eee; /* minimal styling to make it recognizable as btn */
|
||||
padding: 8px; /* vertical padding to fix with host min-height */
|
||||
outline: none; /* focus style handled below, else it follows boundaries of click-area */
|
||||
justify-content: center;
|
||||
}
|
||||
|
||||
:host .btn ::slotted(button) {
|
||||
:host ::slotted(button) {
|
||||
position: absolute;
|
||||
top: 0;
|
||||
left: 0;
|
||||
|
|
@ -98,41 +98,36 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
|
|||
border: 0; /* reset default agent styles */
|
||||
}
|
||||
|
||||
.click-area {
|
||||
position: absolute;
|
||||
top: 0;
|
||||
right: 0;
|
||||
bottom: 0;
|
||||
left: 0;
|
||||
margin: 0;
|
||||
padding: 0;
|
||||
}
|
||||
|
||||
:host(:focus:not([disabled])) .btn {
|
||||
/* Show focus styles on keyboard focus. */
|
||||
:host(:focus:not([disabled])),
|
||||
:host(:focus-visible) {
|
||||
/* if you extend, please overwrite */
|
||||
outline: 2px solid #bde4ff;
|
||||
}
|
||||
|
||||
:host(:hover) .btn {
|
||||
/* Hide focus styles if they're not needed, for example,
|
||||
when an element receives focus via the mouse. */
|
||||
:host(:focus:not(:focus-visible)) {
|
||||
outline: 0;
|
||||
}
|
||||
|
||||
:host(:hover) {
|
||||
/* if you extend, please overwrite */
|
||||
background: #f4f6f7;
|
||||
}
|
||||
|
||||
:host(:active) .btn, /* keep native :active to render quickly where possible */
|
||||
:host([active]) .btn /* use custom [active] to fix IE11 */ {
|
||||
:host(:active), /* keep native :active to render quickly where possible */
|
||||
:host([active]) /* use custom [active] to fix IE11 */ {
|
||||
/* if you extend, please overwrite */
|
||||
background: gray;
|
||||
}
|
||||
|
||||
:host([disabled]) {
|
||||
pointer-events: none;
|
||||
}
|
||||
|
||||
:host([hidden]) {
|
||||
display: none;
|
||||
}
|
||||
|
||||
:host([disabled]) .btn {
|
||||
:host([disabled]) {
|
||||
pointer-events: none;
|
||||
/* if you extend, please overwrite */
|
||||
background: lightgray;
|
||||
color: #adadad;
|
||||
|
|
@ -213,14 +208,9 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
|
|||
* @param {Event} e
|
||||
*/
|
||||
__clickDelegationHandler(e) {
|
||||
if ((this.type === 'submit' || this.type === 'reset') && e.target === this) {
|
||||
if (this._form) {
|
||||
const nativeButton = document.createElement('button');
|
||||
nativeButton.type = this.type;
|
||||
this._form.appendChild(nativeButton);
|
||||
nativeButton.click();
|
||||
this._form.removeChild(nativeButton);
|
||||
}
|
||||
if ((this.type === 'submit' || this.type === 'reset') && e.target === this && this._form) {
|
||||
e.stopImmediatePropagation();
|
||||
this._nativeButtonNode.click();
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -240,6 +230,7 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
|
|||
this.removeEventListener('mousedown', this.__mousedownHandler);
|
||||
this.removeEventListener('keydown', this.__keydownHandler);
|
||||
this.removeEventListener('keyup', this.__keyupHandler);
|
||||
this.removeEventListener('click', this.__clickDelegationHandler);
|
||||
}
|
||||
|
||||
__mousedownHandler() {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { browserDetection } from '@lion/core';
|
||||
import { aTimeout, expect, fixture, html, oneEvent } from '@open-wc/testing';
|
||||
import { aTimeout, expect, fixture, html, oneEvent, unsafeStatic } from '@open-wc/testing';
|
||||
import sinon from 'sinon';
|
||||
import '@lion/core/src/differentKeyEventNamesShimIE.js';
|
||||
import '../lion-button.js';
|
||||
|
|
@ -206,7 +206,7 @@ describe('lion-button', () => {
|
|||
const wrapperId = el.getAttribute('aria-labelledby');
|
||||
expect(/** @type {ShadowRoot} */ (el.shadowRoot).querySelector(`#${wrapperId}`)).to.exist;
|
||||
expect(/** @type {ShadowRoot} */ (el.shadowRoot).querySelector(`#${wrapperId}`)).dom.to.equal(
|
||||
`<div id="${wrapperId}"><slot></slot></div>`,
|
||||
`<div class="button-content" id="${wrapperId}"><slot></slot></div>`,
|
||||
);
|
||||
browserDetectionStub.restore();
|
||||
});
|
||||
|
|
@ -397,6 +397,16 @@ describe('lion-button', () => {
|
|||
});
|
||||
|
||||
describe('click event', () => {
|
||||
/**
|
||||
* @param {HTMLButtonElement | LionButton} el
|
||||
*/
|
||||
async function prepareClickEvent(el) {
|
||||
setTimeout(() => {
|
||||
el.click();
|
||||
});
|
||||
return oneEvent(el, 'click');
|
||||
}
|
||||
|
||||
it('is fired once', async () => {
|
||||
const clickSpy = /** @type {EventListener} */ (sinon.spy());
|
||||
const el = /** @type {LionButton} */ (await fixture(
|
||||
|
|
@ -412,16 +422,25 @@ describe('lion-button', () => {
|
|||
expect(clickSpy).to.have.been.calledOnce;
|
||||
});
|
||||
|
||||
describe('native button behavior', async () => {
|
||||
/**
|
||||
* @param {HTMLButtonElement | LionButton} el
|
||||
*/
|
||||
async function prepareClickEvent(el) {
|
||||
setTimeout(() => {
|
||||
el.click();
|
||||
it('is fired one inside a form', async () => {
|
||||
const formClickSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault()));
|
||||
const el = /** @type {HTMLFormElement} */ (await fixture(
|
||||
html`<form @click="${formClickSpy}">
|
||||
<lion-button>foo</lion-button>
|
||||
</form>`,
|
||||
));
|
||||
|
||||
// @ts-ignore
|
||||
el.querySelector('lion-button').click();
|
||||
|
||||
// trying to wait for other possible redispatched events
|
||||
await aTimeout(0);
|
||||
await aTimeout(0);
|
||||
|
||||
expect(formClickSpy).to.have.been.calledOnce;
|
||||
});
|
||||
return oneEvent(el, 'click');
|
||||
}
|
||||
|
||||
describe('native button behavior', async () => {
|
||||
/** @type {Event} */
|
||||
let nativeButtonEvent;
|
||||
/** @type {Event} */
|
||||
|
|
@ -450,12 +469,43 @@ describe('lion-button', () => {
|
|||
expect(lionButtonEvent[property]).to.equal(nativeButtonEvent[property]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it('has host in the target property', async () => {
|
||||
describe('event target', async () => {
|
||||
it('is host by default', async () => {
|
||||
const el = /** @type {LionButton} */ (await fixture('<lion-button>foo</lion-button>'));
|
||||
const event = await prepareClickEvent(el);
|
||||
expect(event.target).to.equal(el);
|
||||
});
|
||||
|
||||
const useCases = [
|
||||
{ container: 'div', type: 'submit', targetHost: true },
|
||||
{ container: 'div', type: 'reset', targetHost: true },
|
||||
{ container: 'div', type: 'button', targetHost: true },
|
||||
{ container: 'form', type: 'submit', targetHost: false },
|
||||
{ container: 'form', type: 'reset', targetHost: false },
|
||||
{ container: 'form', type: 'button', targetHost: true },
|
||||
];
|
||||
|
||||
useCases.forEach(useCase => {
|
||||
const { container, type, targetHost } = useCase;
|
||||
const targetName = targetHost ? 'host' : 'native button';
|
||||
it(`is ${targetName} with type ${type} and it is inside a ${container}`, async () => {
|
||||
const clickSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault()));
|
||||
const el = /** @type {LionButton} */ (await fixture(
|
||||
`<lion-button type="${type}">foo</lion-button>`,
|
||||
));
|
||||
const tag = unsafeStatic(container);
|
||||
await fixture(html`<${tag} @click="${clickSpy}">${el}</${tag}>`);
|
||||
const event = await prepareClickEvent(el);
|
||||
|
||||
if (targetHost) {
|
||||
expect(event.target).to.equal(el);
|
||||
} else {
|
||||
expect(event.target).to.equal(el._nativeButtonNode);
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue