Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
| const containingBlock = getOffsetParent(this); | ||
| this.inert = false; | ||
|
|
||
| if (containingBlock.dataset.initiallyInert !== '1') containingBlock.removeAttribute('inert'); | ||
| } | ||
| #show() { | ||
| this._state = reduceMotion ? 'shown' : 'showing'; | ||
|
|
||
| const containingBlock = getOffsetParent(this); | ||
|
|
||
| if (containingBlock.getAttribute('inert') !== null) containingBlock.dataset.initiallyInert = '1'; | ||
|
|
||
| containingBlock.setAttribute('inert', 'inert'); | ||
| this.inert = true; |
There was a problem hiding this comment.
This simplification is a nice bonus of this approach 😸
There was a problem hiding this comment.
I would still test the presence of the inert attribute and set initiallyInert on this
There was a problem hiding this comment.
I'm not sure I follow- why would we need that?
There was a problem hiding this comment.
It might be a very niche use case but it's possible the consumer of the component sets inert, then the component enters it's loading/shown state for a second, then inert would be set to false after. This would be activating a component that was made inert externally
There was a problem hiding this comment.
Right, good point. I'll reinstate this check.
There was a problem hiding this comment.
In fact, it might be worth setting inert in a top-level container inside the shadow root. So the component's inert attribute is not affected
| <d2l-backdrop-loading ?shown=${this.loading}></d2l-backdrop-loading> | ||
| `; | ||
| const slot = html`<slot @slotchange="${this._handleSlotChange}"></slot>`; | ||
| const slotWithBackdrop = this.backdrop ? html`<d2l-backdrop-loading ?shown=${this.loading}>${slot}</d2l-backdrop-loading>` : slot; |
There was a problem hiding this comment.
The key change here- slot is a child of d2l-backdrop-loading, not a sibling.
There was a problem hiding this comment.
I might be missing something obvious but could you not render the backdrop-loading component conditionally based on loading? It seems like backdrop and loading are doing the same thing, rendering the backdrop if both are set and only the slotted content otherwise. I understand this is to reduce the surface area of the change, but if it's all behind loading it feel safe, unless there's already cases in the wild that are using it.
There was a problem hiding this comment.
Would that be rendered on the backdrop-loading? I feel like the backdrop property could be introduced with that change(e.g. a slot for the panel)
There was a problem hiding this comment.
The visibility of the backdrop component and the loading property are not in perfect sync due to their animations- even after the loading prop is set to false, the backdrop fades out before ceasing to render completely. The backdrop itself owns those animations, so the table wrapper doesn't know when to take the backdrop out of the DOM tree.
There was a problem hiding this comment.
That said- we could keep state a little differently here and record whether or not loading has ever been true, instead of having the downstream consumer declare it pre-emptively. I don't have a strong opinion either way on that.
| } | ||
|
|
||
| render() { | ||
| if (this._state === 'hidden') return nothing; |
There was a problem hiding this comment.
We always render the component now, though when the state is hidden we're just rendering the slot content and nothing else.
| static get styles() { | ||
| return css` | ||
| :host { | ||
| position: relative; |
There was a problem hiding this comment.
Using position: relative here keeps the backdrop in the document flow, which is necessary for the parent scroll wrapper to properly cut off the width. Before, we had position: absolute and let the sibling slot with the table push out the parent's width.
| position: relative; | ||
| } | ||
|
|
||
| #backdrop-styling-wrapper { |
There was a problem hiding this comment.
What was previously just :host is now assigned to this special ID, which contains the old content of the component prior to this addition.
e7cbd02 to
39353b7
Compare
| @d2l-selection-action-click="${this._toggleLoading}" | ||
| ></d2l-selection-action> | ||
| ${ | ||
| this.backdrop ? html` |
There was a problem hiding this comment.
This button is no longer universal- only tables which explicitly opted in via backdrop can use loading
| * Whether or not to render a loading backdrop. Set this property when the content in the table is being refreshed. | ||
| * @type {boolean} | ||
| */ | ||
| backdrop: { |
There was a problem hiding this comment.
As mentioned in the description, this is strictly defensive to minimize the risk to the LMS of shipping changes here. We can have more discussions about the ideal API at a later date.
https://desire2learn.atlassian.net/browse/NTNGL-5471?atlOrigin=eyJpIjoiMTIyODVmNjAyOGEyNGQ1ZmI2YzBkY2YzNGE5ZDNkMDAiLCJwIjoiaiJ9
TL;DR: Making the containing parent of the loading backdrop inert isn't precise enough. Instead, we should make the backdrop itself inert and place the content under it via a
slotelement.Problem
While working towards the addition of an "Apply" button while in a dirty state to refresh data, I ran into an issue regarding how the
inertproperty was distributed on the table.In the original design, the backdrop would search up the DOM tree looking for a relatively positioned ancestor which it would then make
inert, causing all children to becomeinertand remove them from the accessibility tree as desired.This worked well for the one user of

loading, the insights platform that we build, because we always use the scroll wrapper. On the demo tables however, I noticed that this could bubble up to also make the controls inert if the scroll wrapper wasn't present:It also causes issues when trying to build the apply button into the
backdrop-loadingcomponent- since everything within the scroll wrapper was inert, the overlaid box which allowed a refresh was also inert, making the button non-interactive.Proposed Solution
Instead of living alongside the content, the backdrop wraps the content and is responsible for applying the
inertproperty onto the inner content (in this case, the table). For now, it does this by making itself inert, but in follow up PRs in which the backdrop may inherit the responsibility of surfacing a refresh button, it'll likely apply it to one of its children.Unlike the prior solution, this means that the backdrop is always included in the DOM, even when the loading state isn't enabled. To prevent this change from unnecessarily increasing the blast radius of the backdrop-loading feature, I've added an opt-in table prop which prevents the
backdropcode from being included at all when not included.Demo
Nothing has changed about the functionality of this component- but for posterity, I've uploaded demos anyway.
Demo with backdrop enabled:
https://github.com/user-attachments/assets/a4f5f74c-ef9e-4205-a829-604ac6a40a58
Insights Platform downstream usage:
https://github.com/user-attachments/assets/f32db48a-fbfc-47a3-8194-a49c9ab7aac7