From 488ba6a0bf9d20f8ad514f5b62437a928a08b775 Mon Sep 17 00:00:00 2001 From: Erik Koopmans Date: Fri, 6 Jan 2023 01:14:31 -0500 Subject: [PATCH 1/5] fix: OPL separators on hidden items and edge cases --- .../demo/object-property-list.html | 13 +++++++ .../object-property-list-item.js | 27 +++++++++++++- .../object-property-list.js | 20 ++++++++-- .../test/object-property-list.test.js | 37 +++++++++++++++++-- .../object-property-list.visual-diff.html | 9 +++++ .../test/object-property-list.visual-diff.js | 1 + 6 files changed, 98 insertions(+), 9 deletions(-) diff --git a/components/object-property-list/demo/object-property-list.html b/components/object-property-list/demo/object-property-list.html index d9e0db65d7a..adf6678987a 100644 --- a/components/object-property-list/demo/object-property-list.html +++ b/components/object-property-list/demo/object-property-list.html @@ -59,6 +59,19 @@

Object Property List: Skeleton item count set

+

Object Property List: Hidden items

+ + + + +

Object Property List: No external whitespace (still contains internal whitespace in the component renderers)

diff --git a/components/object-property-list/object-property-list-item.js b/components/object-property-list/object-property-list-item.js index 6ffe9acc004..ef68460f9b0 100644 --- a/components/object-property-list/object-property-list-item.js +++ b/components/object-property-list/object-property-list-item.js @@ -11,6 +11,10 @@ import { SkeletonMixin } from '../skeleton/skeleton-mixin.js'; export class ObjectPropertyListItem extends SkeletonMixin(LitElement) { static get properties() { return { + /** + * @ignore + */ + hidden: { type: Boolean }, /** * Name of an optional icon to display * @type {string} @@ -21,6 +25,7 @@ export class ObjectPropertyListItem extends SkeletonMixin(LitElement) { * @type {string} */ text: { type: String }, + _showSeparator: { state: true }, }; } @@ -29,6 +34,9 @@ export class ObjectPropertyListItem extends SkeletonMixin(LitElement) { :host { vertical-align: middle; } + :host([hidden]) { + display: none; + } d2l-icon { height: 1.2857em; /* 18px desired height at main font size (14px), but using em to scale properly at smaller breakpoint. */ width: 1.2857em; @@ -57,6 +65,11 @@ export class ObjectPropertyListItem extends SkeletonMixin(LitElement) { `]; } + constructor() { + super(); + this._showSeparator = true; + } + render() { return html` ${this._renderIcon()} @@ -65,17 +78,27 @@ export class ObjectPropertyListItem extends SkeletonMixin(LitElement) { `; } + updated(changedProperties) { + super.updated(changedProperties); + if (changedProperties.has('hidden')) this._onHidden(); + } + + _onHidden() { + /** Dispatched when the visibility of the item changes */ + this.dispatchEvent(new CustomEvent('d2l-object-property-list-item-visibility-change', { bubbles: true, composed: true })); + } + _renderIcon() { return this.icon && !this.skeleton ? html`` : nothing; } _renderSeparator() { - return html` + return this._showSeparator ? html` - `; + ` : nothing; } _renderText() { diff --git a/components/object-property-list/object-property-list.js b/components/object-property-list/object-property-list.js index 278f7635feb..f3eac6727df 100644 --- a/components/object-property-list/object-property-list.js +++ b/components/object-property-list/object-property-list.js @@ -28,9 +28,6 @@ class ObjectPropertyList extends LocalizeCoreElement(SkeletonMixin(LitElement)) :host([hidden]) { display: none; } - ::slotted(:last-child), slot :last-child { - --d2l-object-property-list-item-separator-display: none; - } ::slotted([slot="status"]) { display: none; } @@ -41,6 +38,11 @@ class ObjectPropertyList extends LocalizeCoreElement(SkeletonMixin(LitElement)) `]; } + firstUpdated() { + this._slot = this.shadowRoot.querySelector('slot:not([name])'); + this.addEventListener('d2l-object-property-list-item-visibility-change', this._onItemsChanged); + } + render() { const slotContents = this.skeleton && this.skeletonCount > 0 ? [...Array(this.skeletonCount)].map(() => html` @@ -50,10 +52,20 @@ class ObjectPropertyList extends LocalizeCoreElement(SkeletonMixin(LitElement))
- ${slotContents} + ${slotContents}
`; } + + _onItemsChanged() { + this._setItemSeparatorVisibility(); + } + + _setItemSeparatorVisibility() { + const items = this._slot.assignedNodes().filter(item => item.tagName?.toLowerCase().includes('d2l-object-property-list-') && !item.hidden); + const lastIndex = items.length - 1; + items.forEach((item, i) => item._showSeparator = (i !== lastIndex)); + } } customElements.define('d2l-object-property-list', ObjectPropertyList); diff --git a/components/object-property-list/test/object-property-list.test.js b/components/object-property-list/test/object-property-list.test.js index 897fb7d17ce..a2df0a453da 100644 --- a/components/object-property-list/test/object-property-list.test.js +++ b/components/object-property-list/test/object-property-list.test.js @@ -6,12 +6,12 @@ import { aTimeout, expect, fixture, html } from '@open-wc/testing'; import { runConstructor } from '../../../tools/constructor-test-helper.js'; const validateSeparators = (elem, count) => { - const items = elem.querySelectorAll('d2l-object-property-list-item, d2l-object-property-list-item-link'); + const items = elem.querySelectorAll('d2l-object-property-list-item:not([hidden]), d2l-object-property-list-item-link:not([hidden])'); expect(items.length).to.equal(count); items.forEach((item, i) => { - const expectedDisplay = i === items.length - 1 ? 'none' : 'inline'; + const shouldHaveSeparator = i !== items.length - 1; const separator = item.shadowRoot.querySelector('.separator'); - expect(window.getComputedStyle(separator).display).to.equal(expectedDisplay); + expect(!!separator).to.equal(shouldHaveSeparator); }); }; @@ -65,6 +65,37 @@ describe('d2l-object-property-list', () => { await aTimeout(); // Required for Webkit only. validateSeparators(elem, 2); }); + + it('should display correctly if status slot is after all items', async() => { + const elem = await fixture(html` + + + + + + `); + validateSeparators(elem, 2); + }); + + it('should respond to hidden items', async() => { + const elem = await fixture(html` + + + + + + + `); + validateSeparators(elem, 2); + + elem.querySelector('#hidden').removeAttribute('hidden'); + await elem.updateComplete; + validateSeparators(elem, 3); + + elem.querySelector('#hidden').setAttribute('hidden', ''); + await elem.updateComplete; + validateSeparators(elem, 2); + }); }); }); diff --git a/components/object-property-list/test/object-property-list.visual-diff.html b/components/object-property-list/test/object-property-list.visual-diff.html index 09eb2ecb520..fa1c6d6d613 100644 --- a/components/object-property-list/test/object-property-list.visual-diff.html +++ b/components/object-property-list/test/object-property-list.visual-diff.html @@ -69,5 +69,14 @@ +
+ + + + + + +
+ diff --git a/components/object-property-list/test/object-property-list.visual-diff.js b/components/object-property-list/test/object-property-list.visual-diff.js index 9b899d0dddc..8fec1e2f6b8 100644 --- a/components/object-property-list/test/object-property-list.visual-diff.js +++ b/components/object-property-list/test/object-property-list.visual-diff.js @@ -24,6 +24,7 @@ describe('d2l-object-property-list', () => { { name: 'rtl', selector: '#rtl' }, { name: 'list-skeleton', selector: '#list-skeleton' }, { name: 'item-skeleton', selector: '#item-skeleton' }, + { name: 'hidden-items', selector: '#hidden-items' }, ].forEach((info) => { it(info.name, async function() { const rect = await visualDiff.getRect(page, info.selector); From 0d157334904eeb7ffa139fe52eb463c9640fc8c4 Mon Sep 17 00:00:00 2001 From: Erik Koopmans Date: Thu, 19 Jan 2023 16:41:20 -0500 Subject: [PATCH 2/5] Fix separator visibility on skeleton --- .../object-property-list/object-property-list.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/components/object-property-list/object-property-list.js b/components/object-property-list/object-property-list.js index f3eac6727df..51b0c5d4e35 100644 --- a/components/object-property-list/object-property-list.js +++ b/components/object-property-list/object-property-list.js @@ -41,6 +41,7 @@ class ObjectPropertyList extends LocalizeCoreElement(SkeletonMixin(LitElement)) firstUpdated() { this._slot = this.shadowRoot.querySelector('slot:not([name])'); this.addEventListener('d2l-object-property-list-item-visibility-change', this._onItemsChanged); + if (this._slot.childElementCount) this._onItemsChanged(); } render() { @@ -62,9 +63,12 @@ class ObjectPropertyList extends LocalizeCoreElement(SkeletonMixin(LitElement)) } _setItemSeparatorVisibility() { - const items = this._slot.assignedNodes().filter(item => item.tagName?.toLowerCase().includes('d2l-object-property-list-') && !item.hidden); - const lastIndex = items.length - 1; - items.forEach((item, i) => item._showSeparator = (i !== lastIndex)); + const slottedElements = this._slot.assignedElements(); + const elements = slottedElements.length ? slottedElements : [ ...this._slot.children ]; + const filtered = elements.filter(item => item.tagName?.toLowerCase().includes('d2l-object-property-list-') && !item.hidden); + + const lastIndex = filtered.length - 1; + filtered.forEach((item, i) => item._showSeparator = (i !== lastIndex)); } } From c0359b8907a12b24dc0c4372d2ab70b49ae2139a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 19 Jan 2023 16:55:49 -0500 Subject: [PATCH 3/5] Updating Visual Diff Goldens (#3185) Co-authored-by: github-actions --- .../d2l-object-property-list-hidden-items.png | Bin 0 -> 3122 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 components/object-property-list/test/screenshots/ci/golden/object-property-list/d2l-object-property-list-hidden-items.png diff --git a/components/object-property-list/test/screenshots/ci/golden/object-property-list/d2l-object-property-list-hidden-items.png b/components/object-property-list/test/screenshots/ci/golden/object-property-list/d2l-object-property-list-hidden-items.png new file mode 100644 index 0000000000000000000000000000000000000000..d612c1a61edfa929129b890d3bbfafa8f7254530 GIT binary patch literal 3122 zcmd5;`8S)}8vaUYPg^=1^LWoetJUaW)`~jXw23+9Btp%i=Ba3K^x#wv&N4K|tQ0kq zh%_QX71hBMGeIvkgrI~H#Qpjw++XgxzwCFfcdd8tcdz}t&+|O9G`}x$O6C**03z@Q zcaZ=f5C`rpPYQ$Uvq$e#z;G-CdH*+{wpVT$08X#M?;6=W&f~BmZL-1?y1(_&t3``p z-oF;W?`0xULsNaGxY^_}-HJ#UQ~I7}ZeH2Y0FwF0`H?sTZORmryh~KdeyHb>ZQ||o ztGbb7NE&{tSMj)#K>WL})QuMB#X!-d#fZYi{k*EheLaTPp5tmb%?a$`x=7W45EzTX z&jZH*z%&s!2>>oPkKX}v8NUDm0PxS5W9I;%&HZE?n7U{T90!Y}1^xv9aXsh$_a(J3 z$v9qs+oy{Vyl=L$55I-V9;9Zpi%YBN=-8v@3BH}*$vDLud$_xMkF0jPlg8&coSOH4 zK0`HWB7E&?hDa8M73P$gi|~n^-9SdZ{>0Uw|8 zztr1b+Qq(sdG+OIBqb#kpx)du?nr#!*JmLjt8LRQDz6zkjUk^tlc`)YBUSSD?Qc)j zgn=RZMBz5x^v3gXPB6s~wfR)$07n)Y9yuQIM>U^T1w(~| zg!H4=@&-I~O{1a`Cn%?fKI_c9qID$M>8|ls@Q4_OyH}YMFe{a4AcUEbjcHg4TiJGe zD+(Vph?TDma_d&T+=C&s)-6mLM02M%#StbZ87zN-vcVpgRN8gdbT=u{>3mKM0&WFf z%^4?AmWY)!nt?$Yy2!2bBrcbc~tJ#OUWQCi0ZAI=Sfl&Hfs6!bH z>LkV3RoTXT&b420005h^5kZ$Q*;bqHIjzbGBGT1@C(c$z)L~`%4I3x+E9!!SgX{LN z{UWk%jjQ<)CM=We!ooWG{*Lc@>mhq`(cD0jW8_#`NZNLCEuM~!O+Z>(oA6_H zyA`*`@*<=7<6iS$zm{f`6o<1lybw)=dLgiepr8r5SDFNUBd>){uZnE4CuC%lY|CmR z(%0CN_Wu6%m>Z?dlU#aB3-{eYch$C|#?mk;NxS<|zs2RtIZuuR(_dvX z6|L$D3=mxyetuUh(JVkc8l65=<7=NH7nFu#?cVoToLBwjVbZh{zZJ^jXQOuae7N+I z4|3}X1T2zSW=5Xd_!Ge(FM%iSF%IjB)D^?@1F$Fg@G{TNDA8fBqJLyrW6Jj*S z8@a9?r13`AuUE`c|9ai9)pt#`e=lT_>-6tqHP2G*5Qxh7_zB;G-F@7u6aZi<-2eO zJ~$D!l37rIU3lDS#t11l-`w21;=@x{SLb)E3>;wL*SfiqOxgUd@yH@juJnMC@}^B?LW}qF#q+irvX35{b%i z>x&jwB1}hbHLKS9p}#IHWbLPOpb^uK7x>A0JYMB+iVh+jC5+kj?%maAi~np z()xHFcU?a%d0tY?yYqwX#O|b^F#zZ$GwZ-f&5iL>&Doo@`?8ZZM_GP9FpsK!9ksJ= zk*Q36v>ZEpLK#KzSE*RK8Ed<@D`tR?HYRy`covloS}c~8d)Y)XvK;A~cu1^Q_QGv_ zbVTTKF_#-k-&%`CwYdAF$O{5-YI=vLw$VXunzG9BMw;P^<|h$aP$*PCClB+rY^K_g zD-dU-P$KPDDzH|PKdV)Th2U9I$rS zS|w3h5qHkbqRZds{8;Se-$ zrLFxbaeRtdjWf8$S(soZs$fIk1ad9Q3(<(h#UC5c*ECK*p zo%Mi2R^qi`PT9JtaAKl2&ixO8iN+V;Jl-}i5brM|m%(7VovSn3Q0V4{YV$l*Z%rv* zHxhY2Re8Dd-nEsf0ACobxf#bk%uo@BT-J3PdBho~*-K|Tke$x>K{N zFDSu9=q+M5g5qBlXSpamajC>Q%ayJ(j*QZJv+dXTKh!p^=@=OqZJVN?g=An4Y{3Tf zgO%YLd=1(ssjA`1pzkSGW3%nTHYb7Wr#l2(Y(d?jv3eL!{I~fyAf!(5PMVaCq&V+QRggD9JYL1-r%V~P_atQ>@?Kqnh%6}{5cvV8n zcf^LmimFN!l|}~7r~yE#>ymYwt=}$^<;P18?eVYMFo+MQ(T8q^gyDOrn`cfdR)Qp` zCcn6_P`2NdI)HcUw$UE}9gFp^Roc1BYiqS>u5PvZp1ma=UUTZBH7oL2YbBNOx56#K zj Date: Thu, 19 Jan 2023 18:39:15 -0500 Subject: [PATCH 4/5] Remove redundant CSS variable --- components/object-property-list/object-property-list-item.js | 1 - 1 file changed, 1 deletion(-) diff --git a/components/object-property-list/object-property-list-item.js b/components/object-property-list/object-property-list-item.js index ef68460f9b0..ff1dea51fe4 100644 --- a/components/object-property-list/object-property-list-item.js +++ b/components/object-property-list/object-property-list-item.js @@ -42,7 +42,6 @@ export class ObjectPropertyListItem extends SkeletonMixin(LitElement) { width: 1.2857em; } .separator { - display: var(--d2l-object-property-list-item-separator-display, inline); margin: 0 -0.05rem; /* 10px desired margin, subtract 5px arbitrary whitespace and 6px whitespace inside bullet icon. */ } .separator d2l-icon { From 5a80687a956ac4f8d4bfa38a2ed3e8e6c5d4f35d Mon Sep 17 00:00:00 2001 From: Erik Koopmans Date: Fri, 20 Jan 2023 10:54:47 -0500 Subject: [PATCH 5/5] Feedback: Assume slot can change --- .../object-property-list.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/components/object-property-list/object-property-list.js b/components/object-property-list/object-property-list.js index 51b0c5d4e35..a4297da797a 100644 --- a/components/object-property-list/object-property-list.js +++ b/components/object-property-list/object-property-list.js @@ -39,9 +39,10 @@ class ObjectPropertyList extends LocalizeCoreElement(SkeletonMixin(LitElement)) } firstUpdated() { - this._slot = this.shadowRoot.querySelector('slot:not([name])'); - this.addEventListener('d2l-object-property-list-item-visibility-change', this._onItemsChanged); - if (this._slot.childElementCount) this._onItemsChanged(); + this.addEventListener('d2l-object-property-list-item-visibility-change', () => this._onItemsChanged()); + + const slot = this.shadowRoot.querySelector('slot:not([name])'); + if (slot.childElementCount) this._setItemSeparatorVisibility(slot); } render() { @@ -58,13 +59,14 @@ class ObjectPropertyList extends LocalizeCoreElement(SkeletonMixin(LitElement)) `; } - _onItemsChanged() { - this._setItemSeparatorVisibility(); + _onItemsChanged(e) { + const slot = e?.target || this.shadowRoot.querySelector('slot:not([name])'); + this._setItemSeparatorVisibility(slot); } - _setItemSeparatorVisibility() { - const slottedElements = this._slot.assignedElements(); - const elements = slottedElements.length ? slottedElements : [ ...this._slot.children ]; + _setItemSeparatorVisibility(slot) { + const slottedElements = slot.assignedElements(); + const elements = slottedElements.length ? slottedElements : [ ...slot.children ]; const filtered = elements.filter(item => item.tagName?.toLowerCase().includes('d2l-object-property-list-') && !item.hidden); const lastIndex = filtered.length - 1;