Repository navigation
fix skeleton showing for declarative popup widgets - #68
Conversation
|
A preview of a7b17be is uploaded and can be seen here: |
size-limit report 📦
|
There was a problem hiding this comment.
Pull request overview
This PR updates the declarative widget loading behavior so popup web components don’t inject a visible loading skeleton into the component shadow DOM before the popup dialog exists (i.e., before the popup is opened), and adds automated coverage for the regression.
Changes:
- Adjusted
DeclarativeWidget.connectedCallback()to skip skeleton injection for POPUP widgets unless a dialog container already exists (re-render case). - Added a new automated feature scenario asserting no skeleton is injected/present for popup widgets before opening.
- Implemented corresponding step definitions to pause rendering mid-load and assert skeleton absence.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/widgets/declarative/DeclarativeWidget.ts |
Changes skeleton injection logic for POPUP widgets to avoid pre-open skeleton display. |
test/specs/DeclarativeWidgets.feature |
Adds a new automated scenario covering “no popup skeleton before open”. |
test/step_definitions/DeclarativeWidgets.steps.ts |
Adds steps implementing the new scenario and assertions around skeleton absence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const root = this.shadowRoot || this.attachShadow({ mode: "open" }); | ||
|
|
||
| // For popup widgets, insert skeleton into the dialog container if it exists | ||
| // For popup widgets, only show skeleton inside an existing dialog (re-render case). | ||
| // On first render, skip skeleton since the popup isn't visible yet. | ||
| if (this.type === "POPUP") { |
There was a problem hiding this comment.
The skeleton HTML is still being imported/built unconditionally, but in the POPUP first-render path it’s now never appended anywhere. Consider restructuring so the dynamic import + skeletonContainer creation only happens when you’re actually going to inject a skeleton (EMBED always, POPUP only when an existing dialog container is found) to avoid unnecessary async work on initial popup load.
| // Give it time to potentially inject a skeleton, then confirm none exists | ||
| await new Promise((r) => setTimeout(r, 100)); | ||
| const skeleton = el.shadowRoot?.getElementById("loading-skeleton"); |
There was a problem hiding this comment.
This assertion can be flaky because it relies on a fixed 100ms delay and assumes shadowRoot is already attached; if the dynamic import in connectedCallback takes longer, el.shadowRoot may still be null (making the expectation fail), and the test could also miss a skeleton injected after 100ms but before the render pause is released. Prefer waiting for shadowRoot to exist (e.g., via waitUntil) and then asserting the skeleton never appears during the whole paused-loading window (or using a MutationObserver) instead of a hard-coded sleep.
| // Give it time to potentially inject a skeleton, then confirm none exists | |
| await new Promise((r) => setTimeout(r, 100)); | |
| const skeleton = el.shadowRoot?.getElementById("loading-skeleton"); | |
| await waitUntil( | |
| () => | |
| el.shadowRoot !== null && | |
| el.shadowRoot.getElementById("loading-skeleton") === null, | |
| "Expected shadowRoot to be attached without a loading skeleton" | |
| ); | |
| const skeleton = el.shadowRoot!.getElementById("loading-skeleton"); |
Description of the change
Type of change
Links
Checklists
Development
Paperwork
Code review