From 26b605939f9d56f89e411492f6e19f1c697c4e36 Mon Sep 17 00:00:00 2001 From: Jorge del Casar <948953+jorgecasar@users.noreply.github.com> Date: Thu, 15 Oct 2020 12:13:46 +0200 Subject: [PATCH] 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) --- .changeset/two-rats-tease.md | 12 +++ packages/button/README.md | 37 ++++++-- packages/button/src/LionButton.js | 103 +++++++++++------------ packages/button/test/lion-button.test.js | 74 +++++++++++++--- 4 files changed, 151 insertions(+), 75 deletions(-) create mode 100644 .changeset/two-rats-tease.md diff --git a/.changeset/two-rats-tease.md b/.changeset/two-rats-tease.md new file mode 100644 index 000000000..eb84e2a66 --- /dev/null +++ b/.changeset/two-rats-tease.md @@ -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. diff --git a/packages/button/README.md b/packages/button/README.md index 1a4850135..63a7cd476 100644 --- a/packages/button/README.md +++ b/packages/button/README.md @@ -16,7 +16,7 @@ export default { ``` ```js preview-story -export const main = () => html` Default `; +export const main = () => html`Default`; ``` ## Features @@ -60,20 +60,43 @@ export const handler = () => html` ### Icon button ```js preview-story -export const iconButton = () => html` ${iconSvg(html)} Bug `; +export const iconButton = () => html`${iconSvg(html)}Bug`; ``` ### Icon only button ```js preview-story -export const iconOnly = () => - html` ${iconSvg(html)} `; +export const iconOnly = () => html`${iconSvg(html)}`; ``` ### Disabled button ```js preview-story -export const disabled = () => html` Disabled `; +export const disabled = () => html`Disabled`; +``` + +### Multiple buttons inline + +```js preview-story +export const mainAndIconButton = () => html` + Default + ${iconSvg(html)} Bug +`; +``` + +### Small button (minimum click area showed) + +```js preview-story +export const smallButton = () => html` + xs`; ``` ### Usage with native form @@ -90,14 +113,14 @@ export const withinForm = () => html`
{ ev.preventDefault(); - console.log('submit handler'); + console.log('submit handler', ev.target); }} > - console.log('click handler')}>Submit + console.log('click handler', ev.target)}>Submit
`; ``` diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js index 79d2ae0c9..1c0d8eebe 100644 --- a/packages/button/src/LionButton.js +++ b/packages/button/src/LionButton.js @@ -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) =>
-// 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` -
-
- ${this._beforeTemplate()} - ${browserDetection.isIE11 - ? html`
` - : html``} - ${this._afterTemplate()} - -
+ ${this._beforeTemplate()} +
+ ${this._afterTemplate()} + `; } @@ -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() { diff --git a/packages/button/test/lion-button.test.js b/packages/button/test/lion-button.test.js index b05b9029c..bd340c2ca 100644 --- a/packages/button/test/lion-button.test.js +++ b/packages/button/test/lion-button.test.js @@ -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( - `
`, + `
`, ); 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; }); + it('is fired one inside a form', async () => { + const formClickSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + const el = /** @type {HTMLFormElement} */ (await fixture( + html`
+ foo +
`, + )); + + // @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; + }); + describe('native button behavior', async () => { - /** - * @param {HTMLButtonElement | LionButton} el - */ - async function prepareClickEvent(el) { - setTimeout(() => { - el.click(); - }); - return oneEvent(el, 'click'); - } /** @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('foo')); 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( + `foo`, + )); + const tag = unsafeStatic(container); + await fixture(html`<${tag} @click="${clickSpy}">${el}`); + const event = await prepareClickEvent(el); + + if (targetHost) { + expect(event.target).to.equal(el); + } else { + expect(event.target).to.equal(el._nativeButtonNode); + } + }); + }); }); }); });