From 8b3e4e77d6aeb00a80c0428b499e98678dc60042 Mon Sep 17 00:00:00 2001 From: Ruben Carvalho Date: Thu, 10 Oct 2024 11:32:09 +0200 Subject: [PATCH 1/7] fix(dialog): update grid-template-areas to use heading instead of unsused header and typeIcon --- packages/dialog/src/dialog.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dialog/src/dialog.css b/packages/dialog/src/dialog.css index 936d773b7f..fb98c79b6f 100644 --- a/packages/dialog/src/dialog.css +++ b/packages/dialog/src/dialog.css @@ -48,7 +48,7 @@ governing permissions and limitations under the License. grid-template-areas: 'hero hero hero hero hero hero' '. . . . . .' - '. heading header header typeIcon .' + '. heading heading heading heading .' '. divider divider divider divider .' '. content content content content .' '. footer footer buttonGroup buttonGroup .' From 80e705c814a60f8d40bc0f9ced50ee6c709b380a Mon Sep 17 00:00:00 2001 From: Ruben Carvalho Date: Thu, 10 Oct 2024 11:44:48 +0200 Subject: [PATCH 2/7] chore: add storybook story to catch regressions --- .../dialog/stories/dialog-wrapper.stories.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/dialog/stories/dialog-wrapper.stories.ts b/packages/dialog/stories/dialog-wrapper.stories.ts index 8f89190567..6392525d41 100644 --- a/packages/dialog/stories/dialog-wrapper.stories.ts +++ b/packages/dialog/stories/dialog-wrapper.stories.ts @@ -373,6 +373,38 @@ export const longContent = ( longContent.decorators = [isOverlayOpen]; +export const longHeading = ( + args: StoryArgs = {}, + context: { viewMode?: string } = {} +): TemplateResult => { + const open = context.viewMode === 'docs' ? undefined : 'click'; + return html` + + + Content of the dialog + + + Toggle Dialog + + + `; +}; + +longHeading.decorators = [isOverlayOpen]; + export const wrapperDismissableUnderlayError = ( args: StoryArgs = {}, context: { viewMode?: string } = {} From 5fc41c5c99c2f90097fc6a96bb40fb142aca24b8 Mon Sep 17 00:00:00 2001 From: Ruben Carvalho Date: Thu, 10 Oct 2024 15:12:32 +0200 Subject: [PATCH 3/7] chore: add dev warnings and tests --- packages/dialog/src/Dialog.ts | 20 +++++ packages/dialog/src/DialogWrapper.ts | 21 +++++ packages/dialog/src/dialog.css | 2 +- packages/dialog/test/dialog-wrapper.test.ts | 53 +++++++++++++ packages/dialog/test/dialog.test.ts | 85 +++++++++++++-------- 5 files changed, 149 insertions(+), 32 deletions(-) diff --git a/packages/dialog/src/Dialog.ts b/packages/dialog/src/Dialog.ts index 11e2b1975e..18f5a79cae 100644 --- a/packages/dialog/src/Dialog.ts +++ b/packages/dialog/src/Dialog.ts @@ -55,6 +55,9 @@ export class Dialog extends ObserveSlotPresence(AlertDialog, [ @query('.close-button') closeButton?: CloseButton; + /** + * @deprecated Use the Alert Dialog component with `variant="error"` instead. + */ @property({ type: Boolean, reflect: true }) public error = false; @@ -170,4 +173,21 @@ export class Dialog extends ObserveSlotPresence(AlertDialog, [ super.firstUpdated(changes); this.setAttribute('role', 'dialog'); } + + protected override updated(changes: PropertyValues): void { + super.updated(changes); + if ( + changes.has('error') && + (this.error !== undefined || changes.get('error') !== undefined) + ) { + if (window.__swc.DEBUG) { + window.__swc.warn( + this, + `The "error" attribute of <${this.localName}> has been deprecated. Use the Alert Dialog component with the "variant='error'" instead. "error" will be removed in a future release.`, + 'https://opensource.adobe.com/spectrum-web-components/components/alert-dialog/#error', + { level: 'deprecation' } + ); + } + } + } } diff --git a/packages/dialog/src/DialogWrapper.ts b/packages/dialog/src/DialogWrapper.ts index eb6eadb488..01aeac07b3 100644 --- a/packages/dialog/src/DialogWrapper.ts +++ b/packages/dialog/src/DialogWrapper.ts @@ -14,6 +14,7 @@ import { CSSResultArray, html, nothing, + PropertyValues, TemplateResult, } from '@spectrum-web-components/base'; import { property } from '@spectrum-web-components/base/src/decorators.js'; @@ -41,6 +42,9 @@ export class DialogWrapper extends DialogBase { return [...super.styles]; } + /** + * @deprecated Use the Alert Dialog component with `variant="error"` instead. + */ @property({ type: Boolean, reflect: true }) public error = false; @@ -105,6 +109,23 @@ export class DialogWrapper extends DialogBase { ); } + protected override updated(changes: PropertyValues): void { + super.updated(changes); + if ( + changes.has('error') && + (this.error !== undefined || changes.get('error') !== undefined) + ) { + if (window.__swc.DEBUG) { + window.__swc.warn( + this, + `The "error" attribute of <${this.localName}> has been deprecated. Use the Alert Dialog component with the "variant='error'" instead. "error" will be removed in a future release.`, + 'https://opensource.adobe.com/spectrum-web-components/components/alert-dialog/#error', + { level: 'deprecation' } + ); + } + } + } + protected override renderDialog(): TemplateResult { const hideDivider = this.noDivider || diff --git a/packages/dialog/src/dialog.css b/packages/dialog/src/dialog.css index fb98c79b6f..1a1e4fd494 100644 --- a/packages/dialog/src/dialog.css +++ b/packages/dialog/src/dialog.css @@ -48,7 +48,7 @@ governing permissions and limitations under the License. grid-template-areas: 'hero hero hero hero hero hero' '. . . . . .' - '. heading heading heading heading .' + '. heading heading heading typeIcon . ' '. divider divider divider divider .' '. content content content content .' '. footer footer buttonGroup buttonGroup .' diff --git a/packages/dialog/test/dialog-wrapper.test.ts b/packages/dialog/test/dialog-wrapper.test.ts index 6ff3362262..5b642ec015 100644 --- a/packages/dialog/test/dialog-wrapper.test.ts +++ b/packages/dialog/test/dialog-wrapper.test.ts @@ -368,3 +368,56 @@ describe('Dialog Wrapper', () => { expect(contentElement.hasAttribute('tabindex')).to.be.true; }); }); + +describe('dev mode', () => { + let consoleWarnStub!: ReturnType; + before(() => { + window.__swc.verbose = true; + consoleWarnStub = stub(console, 'warn'); + }); + afterEach(() => { + consoleWarnStub.resetHistory(); + }); + after(() => { + window.__swc.verbose = false; + consoleWarnStub.restore(); + }); + + it('warns that `error` is deprecated', async () => { + const el = await styledFixture( + wrapperDismissableUnderlayError() + ); + await elementUpdated(el); + + expect(consoleWarnStub.called).to.be.true; + const dialogWrapperSpyCall = consoleWarnStub.getCall(1); + const dialogSpyCall = consoleWarnStub.getCall(2); + expect( + (dialogWrapperSpyCall.args.at(0) as string).includes('"error"'), + 'confirm error-centric message in dialog wrapper' + ).to.be.true; + expect( + dialogWrapperSpyCall.args.at(-1), + 'confirm `data` shape' + ).to.deep.equal({ + data: { + localName: 'sp-dialog-wrapper', + type: 'api', + level: 'deprecation', + }, + }); + expect( + (dialogSpyCall.args.at(0) as string).includes('"error"'), + 'confirm error-centric message in extended dialog class' + ).to.be.true; + expect(dialogSpyCall.args.at(-1), 'confirm `data` shape').to.deep.equal( + { + data: { + localName: 'sp-dialog', + type: 'api', + level: 'deprecation', + }, + } + ); + }); +}); diff --git a/packages/dialog/test/dialog.test.ts b/packages/dialog/test/dialog.test.ts index 7f67208c3f..3583010f04 100644 --- a/packages/dialog/test/dialog.test.ts +++ b/packages/dialog/test/dialog.test.ts @@ -27,7 +27,7 @@ import { fullscreen, small, } from '../stories/dialog.stories.js'; -import { spy } from 'sinon'; +import { spy, stub } from 'sinon'; import { testForLitDevWarnings } from '../../../test/testing-helpers.js'; describe('Dialog', () => { @@ -156,11 +156,9 @@ describe('Dialog', () => { customElements.define('hero-dialog', Override); - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); const container = el.shadowRoot.querySelector('#hero-container'); expect(container).to.not.be.null; @@ -176,11 +174,9 @@ describe('Dialog', () => { customElements.define('heading-dialog', Override); - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); const container = el.shadowRoot.querySelector('#heading-container'); expect(container).to.not.be.null; @@ -196,11 +192,9 @@ describe('Dialog', () => { customElements.define('content-dialog', Override); - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); const container = el.shadowRoot.querySelector('#content-container'); expect(container).to.not.be.null; @@ -219,11 +213,9 @@ describe('Dialog', () => { customElements.define('footer-dialog', Override); - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); const container = el.shadowRoot.querySelector('#footer-container'); expect(container).to.not.be.null; @@ -242,11 +234,9 @@ describe('Dialog', () => { customElements.define('button-dialog', Override); - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); const container = el.shadowRoot.querySelector('#button-container'); expect(container).to.not.be.null; @@ -262,13 +252,46 @@ describe('Dialog', () => { customElements.define('dismiss-dialog', Override); - const el = await fixture( - html` - - ` - ); + const el = await fixture(html` + + `); const container = el.shadowRoot.querySelector('#dismiss-container'); expect(container).to.not.be.null; }); }); + +describe('dev mode', () => { + let consoleWarnStub!: ReturnType; + before(() => { + window.__swc.verbose = true; + consoleWarnStub = stub(console, 'warn'); + }); + afterEach(() => { + consoleWarnStub.resetHistory(); + }); + after(() => { + window.__swc.verbose = false; + consoleWarnStub.restore(); + }); + + it('warns that `error` is deprecated', async () => { + const el = await fixture(alertError()); + + await elementUpdated(el); + + expect(consoleWarnStub.called).to.be.true; + const spyCall = consoleWarnStub.getCall(0); + expect( + (spyCall.args.at(0) as string).includes('"error"'), + 'confirm error-centric message' + ).to.be.true; + expect(spyCall.args.at(-1), 'confirm `data` shape').to.deep.equal({ + data: { + localName: 'sp-dialog', + type: 'api', + level: 'deprecation', + }, + }); + }); +}); From 3c657bf4aa16a543c072d67519ce701538280ed7 Mon Sep 17 00:00:00 2001 From: Ruben Carvalho Date: Thu, 10 Oct 2024 15:52:01 +0200 Subject: [PATCH 4/7] chore: actually fix the original problem --- packages/dialog/src/dialog.css | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/dialog/src/dialog.css b/packages/dialog/src/dialog.css index 1a1e4fd494..2c35f7ecfc 100644 --- a/packages/dialog/src/dialog.css +++ b/packages/dialog/src/dialog.css @@ -44,11 +44,26 @@ governing permissions and limitations under the License. height: auto; } +/* We may remove this override when CSS fixes unused `header` and `typeIcon` areas */ .grid { grid-template-areas: 'hero hero hero hero hero hero' '. . . . . .' - '. heading heading heading typeIcon . ' + '. heading heading heading heading . ' + '. divider divider divider divider .' + '. content content content content .' + '. footer footer buttonGroup buttonGroup .' + '. . . . . .'; + inline-size: 100%; + display: grid; +} + +/* Needed while `error` attribute still exists as deprecated */ +:host(:not([error])) .grid { + grid-template-areas: + 'hero hero hero hero hero hero' + '. . . . . .' + '. heading heading heading heading . ' '. divider divider divider divider .' '. content content content content .' '. footer footer buttonGroup buttonGroup .' From 27d8cd4328babed0ddfd22bbb3aa29fe3841f746 Mon Sep 17 00:00:00 2001 From: Ruben Carvalho Date: Fri, 11 Oct 2024 10:38:05 +0200 Subject: [PATCH 5/7] chore: oops did not fix the thing I wanted to fix --- packages/dialog/src/dialog.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dialog/src/dialog.css b/packages/dialog/src/dialog.css index 2c35f7ecfc..5b2aa28c24 100644 --- a/packages/dialog/src/dialog.css +++ b/packages/dialog/src/dialog.css @@ -49,7 +49,7 @@ governing permissions and limitations under the License. grid-template-areas: 'hero hero hero hero hero hero' '. . . . . .' - '. heading heading heading heading . ' + '. heading heading heading typeIcon . ' '. divider divider divider divider .' '. content content content content .' '. footer footer buttonGroup buttonGroup .' From 73ab94f1833860a5f1ae7224181acd5531e48be8 Mon Sep 17 00:00:00 2001 From: Ruben Carvalho Date: Fri, 11 Oct 2024 12:00:29 +0200 Subject: [PATCH 6/7] chore: more selectors --- packages/dialog/src/dialog.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dialog/src/dialog.css b/packages/dialog/src/dialog.css index 5b2aa28c24..589c3590e8 100644 --- a/packages/dialog/src/dialog.css +++ b/packages/dialog/src/dialog.css @@ -59,7 +59,7 @@ governing permissions and limitations under the License. } /* Needed while `error` attribute still exists as deprecated */ -:host(:not([error])) .grid { +:host(:not([error], [dismissable], [mode])) .grid { grid-template-areas: 'hero hero hero hero hero hero' '. . . . . .' From 9b4edf23db22ad90aee67c46ffb884b3262f2f5d Mon Sep 17 00:00:00 2001 From: Ruben Carvalho Date: Mon, 14 Oct 2024 14:22:03 +0200 Subject: [PATCH 7/7] chore: update golden images hash --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index d6453aab38..4db8baf4c9 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -10,7 +10,7 @@ executors: parameters: current_golden_images_hash: type: string - default: 3bb8a93772e35ef3bb5e156f76b7c8bd72786144 + default: ef7c7c0e0b5827a2f5e95bf6aba539959453f5dc wireit_cache_name: type: string default: wireit