Loading/dirty state refactoring#7232
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. |
| class BackdropLoading extends DataStateMixin(LocalizeCoreElement(LitElement)) { | ||
|
|
||
| static properties = { | ||
| /** |
There was a problem hiding this comment.
These properties were factored out into DataStateMixin, where the attributes are reflected as we normally would. (i.e. data-state instead of dataState).
| _state: { type: String, reflect: true }, | ||
| _spinnerTop: { state: true }, | ||
| _ariaContent: { state: true } | ||
| _ariaContent: { state: true }, |
There was a problem hiding this comment.
These private reactive properties were re-order and renamed. The _dirtyDialogTop (renamed to _dirtyOverlayTop) should have been a reactive private property - it was renamed and included here as we'd expect.
|
|
||
| static styles = [css` | ||
| #visible { | ||
| .backdrop-container { |
There was a problem hiding this comment.
This is simply renamed for clarity.
| this._spinnerTop = 0; | ||
| this._dirtyDialogTop = 0; | ||
| this._ariaContent = ''; | ||
| this._dirtyOverlayTop = 0; |
| render() { | ||
| const backdropVisible = this._state !== 'hidden'; | ||
|
|
||
| const backdropContainer = backdropVisible ? html`<div class="backdrop-container"> |
There was a problem hiding this comment.
This HTML was just moved out to tidy things up and reduce indentation.
| super.willUpdate(changedProperties); | ||
|
|
||
| if (changedProperties.has('dataState') && changedProperties.get('dataState') !== undefined) { | ||
| this.#clearLiveArea(); |
There was a problem hiding this comment.
These "LiveArea" methods were just renamed for clarity.
| } | ||
| } | ||
|
|
||
| async #centerLoadingSpinnerAndDialog() { |
There was a problem hiding this comment.
Other than renaming, this logic remained exactly the same.
| clearTimeout(this.announcementTimeout); | ||
| } | ||
|
|
||
| this.announcementTimeout = null; |
There was a problem hiding this comment.
Renamed to be private, and to be specific about it being an "Id".
| #fade() { | ||
| let hideImmediately = reduceMotion || this._state === 'showing'; | ||
| if (this._state === 'shown') { | ||
| const currentOpacity = getComputedStyle(this.shadowRoot.querySelector('#d2l-live-region')).opacity; |
There was a problem hiding this comment.
Again, renaming for clarity.
| #getBackdropTarget() { | ||
| const parent = getComposedParent(this); | ||
|
|
||
| const targetedChildren = getComposedChildren( |
There was a problem hiding this comment.
Just tidied up the formatting.
| opacity: 1; | ||
| transition: opacity ${FADE_DURATION_MS}ms ease-in ${SPINNER_DELAY_MS}ms; | ||
| } | ||
| :host([_state="shown"][dataState="dirty"]) d2l-loading-spinner, |
There was a problem hiding this comment.
Then improper property reflection tripped me up during the refactoring because I'd corrected it and didn't realize the CSS was relying on this subtle difference.
| * @type {'clean'|'dirty'|'loading'} | ||
| * @default "clean" | ||
| */ | ||
| dataState: { type: String, attribute: 'data-state', reflect: true }, |
There was a problem hiding this comment.
Note that d2l-table-wrapper previously did not specify the attribute and was therefore defaulting to dataState. This change would normally be breaking, but the only usage is pointing to the JavaScript property here in Insights, so we can safely update to define with the data-state attribute we would normally expect.
1 participant
GAUD-10306
This PR factors out the data state properties (
dataState,dirtyText, anddirtyButtonText) into a mixin since they are used in two places currently, and soond2l-listas well. It also introduces thedataStatesenum (clean,loading,dirty) to eliminate these hard-coded values. This PR also tidies things up with some naming and formatting.I will add comments to make review easier.