Skip to content

GAUD-9445: only render loading backdrop when shown#6601

Open
dlockhart wants to merge 4 commits intomainfrom
GAUD-9445/backdrop-loading-improvements
Open

GAUD-9445: only render loading backdrop when shown#6601
dlockhart wants to merge 4 commits intomainfrom
GAUD-9445/backdrop-loading-improvements

Conversation

@dlockhart
Copy link
Member

When this feature shipped, there was an issue where the loading spinner was getting in the way even when the backdrop-loading wasn't used -- which is the case almost everywhere.

This changes things around so that by default the backdrop will both be display: none and also render nothing. Once it's shown, then it'll render things and start the animations/spinner.

That required some changes to timings, as you can't animate something that's display: none or not rendered! 😆

@github-actions
Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-6601/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

super();
this.shown = false;
this._state = null;
this._state = 'hidden';
Copy link
Member Author

@dlockhart dlockhart Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a new default hidden state.

So the lifecycle now looks like:

  1. hidden: nothing rendered, host is display: none
  2. showing: host and loading spinner are rendered but have opacity: 0
  3. shown: opacity is now set, fade-in animation starts
  4. hiding: opacity is set back to 0, fade-out animation starts
  5. hidden: back to the beginning, nothing is rendered and host is display: none

updated(changedProperties) {
if (changedProperties.has('_state')) {
if (this._state === 'showing') {
setTimeout(() => this._state = 'shown', BACKDROP_DELAY_MS);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the trick/magic here, as we move the 800ms delay from the animation to a timeout. This allows things to render with no visible changes for 800ms, then start the fade-in.

Copy link
Contributor

@duncanuszkay-d2l duncanuszkay-d2l Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool- good idea 👀

One Q- does this get cancelled if the load takes less time than the delay? E.G. does it ever go hidden -> showing -> hidden -> shown and get stuck in that state?

Copy link
Contributor

@duncanuszkay-d2l duncanuszkay-d2l Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I can trigger that scenario if I adjust the load timing on the demo to 50ms from 5000ms:

Recording.2026-02-13.162017.mp4

Though I imagine that's a solvable problem- I'll try adding on a check in the timeout block?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch prevents that issue afaict:

if (this._state === 'showing') {
  setTimeout(() => {
	  if (this._state === 'showing') this._state = 'shown';
  }, BACKDROP_DELAY_MS);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah good call! Yeah I think a check in the timeout block would work, just to make sure it's still in the state it expects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's fixed, good catch. I also needed to just hide immediately in #fade() if shown gets set to false while things are still showing.

@dlockhart dlockhart force-pushed the GAUD-9445/backdrop-loading-improvements branch from 661bb62 to d35bf89 Compare February 13, 2026 20:37
<div class="backdrop" _state=${this._state} @transitionend=${this.#handleTransitionEnd} @transitioncancel=${this.#hide}></div>
<d2l-loading-spinner _state=${this._state} size="${this._state === null ? '0' : '50'}"></d2l-loading-spinner>
<div class="backdrop" @transitionend="${this.#handleTransitionEnd}" @transitioncancel="${this.#hide}"></div>
<d2l-loading-spinner></d2l-loading-spinner>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also removed _state on the backdrop and spinner as we can just use a :host([_state="whatever"]) d2l-loading-spinner selector.

Size also isn't needed anymore since nothing renders at all when the state is hidden.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@dlockhart dlockhart marked this pull request as ready for review February 13, 2026 21:05
@dlockhart dlockhart requested a review from a team as a code owner February 13, 2026 21:05
:host([_state="showing"]),
:host([_state="shown"]),
:host([_state="hiding"]) {
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 CSS noob question- why are we setting this here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see in the W3 schools docs that this uses a flexbox layout which aligns items in a row- but these are laid out more on top of each other 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using flexbox is an alternate (and IMO better) way to center the d2l-loading-spinner horizontally, instead of width: 100%; height: 100% which results in it taking up 100% of the space. Which is... fine, but not what I would have expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, I assume that centers by default because you're using it in conjunction with justify-content:center?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly. There are a few other ways... left: 50% + a transform: translate(-50%, -50%) but I like avoiding transform whenever possible.

Copy link
Contributor

@duncanuszkay-d2l duncanuszkay-d2l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The less rendering, the better 😸 Learned the hard way!

This change looks like an improvement to me, so I'll leave my ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants