Skip to content

Feature 2126 layer loading indicator - #2888

Merged
DarianGill merged 48 commits into
developfrom
feature-2126-layer-loading-indicator
Sep 25, 2026
Merged

DarianGill merged 48 commits into
developfrom
feature-2126-layer-loading-indicator

Conversation

@DarianGill

@DarianGill DarianGill commented Aug 29, 2026 •

Copy link
Copy Markdown
Contributor

What

Closes #2126 by adding a status bar widget at the top-right of the map that shows a loading row beneath the scale bar whenever layers are loading or being rendered for the first time. The row shows a layer name (or a summary count for multiple layers) and collapses automatically once all tracked layers are ready. Layer items in the toolbar also reflect the per-layer loading state through a shared isLoadingLayer flag. An internal LayerLoadingCoordinator module centralizes all logic for computing the aggregate loading state.

Additionally, the feature-restore path is hardened so stale asynchronous waiters expire after a bounded timeout, layer visibility changes are handled correctly (interrupting only the affected feature's restore, restarting when a hidden searchable layer becomes visible again), and error/invisible layers are excluded from restore searches.

Also fixes two related UI polish issues that came up during development: the layer toolbar's loading/error icon and settings button no longer shift neighboring icons when they appear/disappear, and the in-progress draw polygon no longer gets visually buried underneath other vector layers that load in later.

Why

Previously there was no map-wide visual feedback while layers were loading. Users could open the map and see either a blank globe or partial data with no indication that more content was incoming. The layer toolbar's loading spinner was also inconsistent—it only reflected the data-request phase (status === "loading"), not the additional render phase (displayReady === false/tilesLoading === true) that occurs after data arrives.

The feature-restore session also had subtle correctness issues: it could hang indefinitely if no matching feature ever loaded, would retry against invisible or errored layers, and did not restart when the set of visible searchable layers changed between calls.

Two secondary issues surfaced during manual testing: rapid layer status churn made the status bar's loading message flicker between names/counts, and the layer toolbar's status icon/settings button toggling display caused neighboring icons (filter icon, gear button) to jostle horizontally. Separately, because the draw layer's CustomDataSource is created once, early, in DownloadPanelView.initialize(), any other vector layer added or toggled visible afterward could end up stacked above it in Cesium's draw order, burying the user's in-progress polygon.

How (Claude Sonnet 5)

Production files

  • LayerLoadingCoordinator.js — New module. A pure, stateless computation module that extracts all aggregate loading-state logic out of Map.js: determines which layers should be tracked (excludeFromLoadingState), identifies layers that are still loading (status === "loading", or tilesLoading === true when tracked, or the displayReady === false fallback for layer types that don't track tilesLoading, e.g. vector data), formats the user-facing loading message (one label, or "X and N other layer(s)"), truncates long/HTML-bearing labels without breaking markup, syncs the per-layer isLoadingLayer flag, and writes isLoadingLayers/loadingLayersMessage to the map model.
  • MapStatusBarView.js — New view. Composes ScaleBarView with the new LayerLoadingIndicatorView and owns all reveal/collapse timing: waits 1 s before expanding the loading row (so brief loads never flash), keeps it open at least 1 s once expanded, shows a "Loading completed!" message if loading finishes early, and mirrors the scale bar's live pixel width onto a CSS variable so the box grows/shrinks by exactly that amount. Also throttles visible message-text updates via scheduleMessageUpdate()/flushMessageUpdate() to once per MESSAGE_UPDATE_INTERVAL_MS (400 ms), so a burst of near-simultaneous layer status changes settles into one update instead of flickering, while always resolving to the latest message once the window elapses. A fresh loading start/resume (tracked via isCurrentlyLoading) and the one-time "Loading completed!" message both bypass the throttle so meaningful transitions stay immediate; the new timer is cleaned up in handleLoadingFinished()/onClose().
  • LayerLoadingIndicatorView.js — New view. A purely presentational animated bar + message row with no timing logic of its own; sets the message via innerHTML so raw HTML labels (e.g. <sub>/<sup>) render the same as they do in the layer menu.
  • Map.js — Imports LayerLoadingCoordinator and listens on each layer-group collection (change:status/change:displayReady/change:tilesLoading, change:visible, and update/reset) to recompute loading state and re-run feature restore as needed. Adds featureRestoreTimeoutMs (default 15000) to defaults. Hiding a layer mid-restore now clears just that layer's restore entries/selected features instead of the whole session.
  • ActiveFeatureRestoreController.js — Adds a bounded session timeout that calls LayerLoadingCoordinator.updateLayerLoadingState on expiry. Incorporates the current searchable/visible layer ids into the session key (serializeRestoreScopeKey, JSON-serialized to avoid comma-collision) so visibility changes invalidate the existing session. Filters out invisible layers, error-status layers, and non-observable objects from restore searches and waiter registrations. Adds clearFeatureRestoreEntriesForLayer() to surgically remove one layer's restore entries and re-sync the URL.
  • MapAsset.js — Adds displayReady (default null, one-time latch) and tilesLoading (default null, continuously-updated pending-work signal) attributes. resetStatus() now sets status to "loading", displayReady to false, and tilesLoading to null rather than resetting to defaults. Updates the JSDoc for status to include the 'loading' value. Adds startLoadingStateTracking()/stopLoadingStateTracking() no-op hooks for subclasses to override. Adds a renderAboveOtherLayers flag to defaults, used to mark layers (like the in-progress draw polygon) that should always be re-raised above other vector layers as they load in.
  • Cesium3DTileset.js — Back-references the owning MapAsset model on the Cesium primitive (cesiumModel.mapAssetModel). Implements startLoadingStateTracking()/stopLoadingStateTracking(): sets displayReady on the tileset's tileVisible event (one-time), and keeps tilesLoading in sync via the tileset's own loadProgress event (numberOfPendingRequests/numberOfTilesProcessing) for the layer's whole lifetime in the scene.
  • CesiumImagery.js — Back-references the MapAsset model on the Cesium imagery layer. Implements startLoadingStateTracking()/stopLoadingStateTracking(): isDisplayReadyInScene() inspects the imagery cache for a READY/TEXTURE_LOADED tile to set the one-time displayReady latch on postRender. wrapRequestImageForPendingCount() monkey-patches the provider's requestImage to count only genuinely in-flight requests for tilesLoading, avoiding false "still loading" state from Cesium's internal cache retaining stale/orphaned LOD-fallback tiles.
  • CesiumVectorData.js — Fixes a bug where model.setError.bind(...) was never invoked (leaving status: "loading" stuck forever on load failure); now calls model.setError(...) directly. Only resets displayReady during an active load cycle (status === "loading"), not during selection/highlight restyling. Also tags the Cesium CustomDataSource with mapAssetModel so CesiumWidgetView can look up the asset config for a given data source.
  • CesiumWidgetView.js — In add3DTileset/addImagery, calls the asset's startLoadingStateTracking({ scene }) and requests a render. In remove3DTileset/removeImagery, calls stopLoadingStateTracking() to clean up listeners. addVectorData/removeVectorData now track any data sources flagged renderAboveOtherLayers and re-raise them to the top of the dataSourceCollection every time a vector layer is added, so a layer like the draw polygon keeps rendering above layers that load in later. We also now stop tracking for all assets during CesiumWidget teardown and cancel any pending raises after teardown.
  • MapWidgetContainerView.js — renderScaleBar() renamed to renderStatusBar() and now renders MapStatusBarView (passing the map model) in place of a bare ScaleBarView; this.scaleBar renamed to this.statusBar.
  • ScaleBarView.js — Hides the lat/lng labels with visibility instead of display so they keep occupying layout space (preventing shift on mouse enter/leave), and triggers update:barWidth whenever the bar's pixel width changes so MapStatusBarView can mirror it.
  • MapView.js — Passes mapModel to FeatureInfoView and clears the (now-removed) loading indicator timer reference in onClose.
  • LayerItemView.js — showStatus() now reads both status and isLoadingLayer (via new getStatusState()/appendStatusElement() helpers) to decide what to render, so the loading spinner appears whenever the map model marks the layer as loading, not only when status === "loading". Adds a listener on change:isLoadingLayer and re-evaluates status on change:visible. Nulls out statusIcon/badge references in removeStatuses(). Also now creates a fixed-size, always-present .list-item__status-icon slot during render() (before any loading/error state is known); showLoading()/showError() just set innerHTML on that slot and toggle a --visible modifier class, and removeStatuses() clears it back out—the element itself never enters/leaves the DOM, so it no longer jostles the filter icon.
  • FeatureInfoView.js — Calls mapModel.clearFeatureRestoreSession() when the feature info panel is closed, so a dismissed panel doesn't leave a stale restore session active.
  • DownloadPanelView.js — Marks the ephemeral drawing polygon layer with excludeFromLoadingState: true (so it never triggers the loading indicator) and renderAboveOtherLayers: true (so it stays visible above other layers while the user draws).
  • map-view.css — Replaces the old .scale-bar self-positioning box with a new .map-status-bar wrapper that owns position/shadow/background/z-index and an explicit, non-content-driven width (base width + live bar-width CSS variable) so it can animate smoothly with transition: width in every browser. Adds the .map-status-bar__loading-row grid-rows (0fr→1fr) expand/collapse animation, __loading-bar (animated sliding highlight), __loading-message/__loading-text styles. Adds tabular-nums + reserved min-widths on the lat/lng coordinate spans to prevent jitter while panning. Increases .map-view__feature-info-container's reserved top margin (max-height) so a tall panel can't grow up far enough to cover the status bar, and switches it from overflow: hidden to scrollable. Gives .list-item__status-icon a fixed 1rem × 1rem footprint with visibility: hidden by default (reserving the space) and visibility: visible when active. .list-item__settings (the gear/settings button) now stays display: flex at all times and toggles visibility: hidden ↔ visible instead, with a permanent (transparent-when-hidden) 1px border so its box size never changes and the grid column width stays constant regardless of hover state.

Test files

  • MapStatusBarView.spec.js — New spec. Covers initialization/rendering and the full timing state machine with sinon.useFakeTimers(): no expansion before the reveal delay, expansion after the delay, no flash if loading finishes first, minimum-open-duration + "completed" message, immediate collapse after the minimum duration has elapsed, and staying open/updating the message if loading resumes before collapsing.
  • Map.spec.js — Adds a suite of tests covering: feature restore retried when layer loading metadata changes; restore sessions replaced when visible searchable layers change (including comma-containing ids and stale-waiter cancellation); restore state kept alive when an unrelated layer is hidden but cleared/URL-synced when the restoring layer itself is hidden; restore sessions never masquerade as layer loading state; visible loading layers set isLoadingLayers while error/not-yet-display-ready layers are handled correctly; per-layer isLoadingLayer flags stay in sync, including on dynamic add/remove; helper layers with excludeFromLoadingState are ignored; multi-layer loading messages are formatted correctly; labels preserve <sub>/<sup> markup and truncate long labels without breaking markup.
  • MapView.spec.js — Updates the feature-info suite to verify clearFeatureRestoreSession() is called on close; removes the old in-MapView loading indicator timing suite (that DOM now lives in MapStatusBarView).
  • LayerItemView.spec.js — Adds a status rendering suite verifying the loading spinner appears only when isLoadingLayer is true and the layer is visible.
  • MapWidgetContainerView.spec.js — Updates scale-bar tests to assert .map-status-bar presence/absence instead of the old bare .scale-bar container.

Testing

  • 1480 tests passing, 0 failures (PORT=3002 npm test).
  • All new behavior is covered by unit tests in Map.spec.js, MapStatusBarView.spec.js, and LayerItemView.spec.js.
  • Manual: load a map with several tile/imagery layers; the status bar's loading row should expand ~1 s after a layer starts loading and list the loading layer name(s), then collapse (with a brief "Loading completed!" message if it finishes early) once all visible layers have rendered at least one tile/imagery frame.
  • Verify that toggling a layer off while it is still loading removes it from the indicator, and toggling it back on restores it.
  • Verify the drawing polygon layer never appears in the loading message.
  • Verify panning the map (bringing new imagery/3D tiles into view) doesn't cause the indicator to get stuck open or closed.
  • Verify that with several layers rapidly changing loading status in quick succession, the status bar message settles smoothly instead of flickering.
  • Verify the layer toolbar's filter icon and settings/gear button no longer shift position when a layer's loading/error icon appears or disappears.
  • Verify the in-progress draw polygon stays visible above other vector layers even as those layers load in or are toggled on after drawing has started.

Documentation

New public methods carry JSDoc comments with @since 0.0.0 placeholders and new views effectively generate docs on the metacatui api page. JSDoc on MapAsset.status is updated to document the new 'loading' value. Map.js defaults JSDoc includes the new featureRestoreTimeoutMs property.

Adds a layer loading indicator absolutely positioned at the top of the map which contains an ainmated loading bar and describes the layers being loaded. The load message is truncated with ellipses and says +n more when there are >2 layers.

Layer loading listeners are set up in map.js and exclude any layers where the exclueFromLoadingState property is true. This allows the indicator to ignore the "your polygon" layer used by the download tool. The mapview renders and updates the indicator itself, listening to the loading message updates from map.js.

Map assets now have a 'loading' status that allows them to indicate the resource is being requested and triggers a Font Awesome spinner icom in the LayerItemView which renders to the right of the name in the portal Layers menu.
…show the same layers.

Also allows the user to toggle off a layer mid-restore and dismiss the loading widget.

Issue: #2126
… delay only occurs first time reveling the loading indicator.

Issue: #2126
So feature info panel doesn't keep opening and closing when you toggle layers. Toggling off a layer dismisses the selected feature, even if it's mid-restore.

Issue: #2126

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds map-wide and per-layer loading indicators while extending URL-based feature restoration.

Changes:

  • Centralizes layer loading-state tracking and UI feedback.
  • Tracks Cesium render readiness across imagery, vector, and 3D tiles.
  • Hardens asynchronous feature restoration and stable feature IDs.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/js/models/maps/LayerLoadingCoordinator.js Coordinates aggregate loading state.
src/js/models/maps/ActiveFeatureRestoreController.js Manages feature restoration sessions.
src/js/models/maps/Map.js Integrates loading and restoration logic.
src/js/models/maps/featureIdHelpers.js Extracts stable feature IDs.
src/js/models/maps/assets/MapAsset.js Adds display-readiness state.
src/js/models/maps/assets/Cesium3DTileset.js Tracks tile rendering and feature lookup.
src/js/models/maps/assets/CesiumImagery.js Associates imagery with its asset.
src/js/models/maps/assets/CesiumVectorData.js Updates readiness and feature matching.
src/js/common/SearchParams.js Persists feature IDs in URLs.
src/js/views/maps/MapView.js Renders the loading indicator.
src/js/views/maps/LayerItemView.js Shows per-layer loading status.
src/js/views/maps/CesiumWidgetView.js Reports Cesium display readiness.
src/js/views/maps/FeatureInfoView.js Smooths feature-content transitions.
src/js/views/maps/DownloadPanelView.js Excludes drawing layers from tracking.
src/css/map-view.css Styles and positions the indicator.
test/js/specs/unit/models/maps/Map.spec.js Tests loading and restoration behavior.
test/js/specs/unit/common/SearchParams.spec.js Tests feature-ID URL state.
test/js/specs/unit/views/maps/MapView.spec.js Tests delayed indicator behavior.
test/js/specs/unit/views/maps/LayerItemView.spec.js Tests layer loading icons.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/js/models/maps/Map.js Outdated
Comment thread src/js/models/maps/Map.js
Comment thread src/js/models/maps/Map.js
Comment thread src/js/models/maps/ActiveFeatureRestoreController.js Outdated
Comment thread src/js/views/maps/CesiumWidgetView.js Outdated
Comment thread src/js/views/maps/MapView.js Outdated
Removes per-layer listener wiring that didn't support dynamically added layers and reducess stale callback risk from anonymous functions and overall event book keeping. Also adds tests for dynamically added and removed layers ensuring the loading state is recalculated.

Issue: #2126
…reRestoreController.js to avoid comma-joined key collisions.

Also add regression test for such a colission.

Issue: #2126
…apAsset.js so that each layer type reliably has fetched/decoded at least one drawable tile.

Previously we just listened to postRender, which just means that a scene frame completed and it can fire immediately after requestRender() while this imagery layer's network tiles are still pending. That meant displayReady was set too early, dismissing the layer loading indicator before the tiles were actually visible. This refactor allows readiness signaling to live with each asset type.

status: "ready" means “provider created,” not “user can see this layer,” and postRender means “a frame happened,” not “this layer drew imagery.” The refactor makes displayReady mean the latter, which is the value the loading coordinator actually needs.

Issue: #2126

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/js/models/maps/featureIdHelpers.js:20

  • These fallback fields do not uniquely identify a feature. Two features named Roads, or features in different layers with id: "1", serialize to the same f value; restoration then returns whichever matching feature/layer is searched first. Matching against any candidate key makes collisions possible even when one feature has a unique id equal to another feature's name. Encode the owning layer plus an explicit stable property key/value, or restrict persistence to identifiers guaranteed unique within a defined scope.
  const FEATURE_ID_KEYS = [
    "id",
    "identifier",
    "uuid",
    "name",
    "title",
    "label",
  ];

src/css/map-view.css:349

  • This automatically started animation can run indefinitely while the rest of the map remains usable, but it provides no reduced-motion alternative. Respect prefers-reduced-motion so users who disable nonessential motion do not receive a continuously sliding highlight; the text still communicates the loading state.
  animation: map-view-loading-bar 1.5s ease-in-out infinite;

Comment thread src/js/models/maps/Map.js
handleLayerVisibilityChange now runs only for actual visibility changes, and displayReady/status transitions no longer look like visibility=false. This preserves loading UX behavior as
a separate loading-state callback still updates aggregate loading state for status/displayReady/label events.
It also keeps the listener lifecycle clean as
registration and cleanup are symmetric for both callbacks, so no stale listeners or duplicate calls.

Issue: #2126
@DarianGill
DarianGill changed the base branch from develop to feature-2636-url-encode-active-feature September 1, 2026 02:42
Bind visibility restore logic only to change:visible; keep status/displayReady on loading-state callback. Avoid clearing pending feature-restore URL ids when no searchable layers are currently visible
ensure delayed loading indicator has current message text before reveal. Prevent selection-driven CesiumVectorData restyling from toggling displayReady false unless it's actively loading.

Issue: #2126
@DarianGill
DarianGill requested a review from robyngit September 1, 2026 04:32
@DarianGill
DarianGill marked this pull request as ready for review September 1, 2026 04:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread src/js/views/maps/MapView.js Outdated
Comment thread src/js/models/maps/LayerLoadingCoordinator.js
Comment thread src/js/models/maps/Map.js
Comment thread src/js/models/maps/assets/Cesium3DTileset.js Outdated
Comment thread src/js/models/maps/assets/CesiumImagery.js Outdated
Comment thread src/js/models/maps/assets/MapAsset.js
Comment thread src/js/views/maps/MapWidgetContainerView.js
Comment thread src/js/models/maps/ActiveFeatureRestoreController.js
…are unlabeled layers

Old branches used the number of non-empty labels rather than the number of loading layers. If one tracked layer had no label and another is named, it would return only “Loading ” even though both contribute to isLoadingLayers. Instead we now use loadingLayers.length for the single/multiple decision and count.

Issue: #2126
Initializing tilesLoading to true can leave an imagery layer permanently loading when Cesium issues no requestImage call—for example, when the layer rectangle is outside the current view. The wrapper only updates the flag after a promise-returning request starts or settles, so this initial value is never cleared in that case. Initializing it to false prevents this and an actual request still immediately switches it to true.

Issue: #2126
renderStatusBar is explicitly called again when scale, mousePosition, or interactions is replaced, which doesn't currently happen but is good defensive programming. It used to append a new status bar without closing or removing the previous one, so the old bar would remain visible and subscribed to the map, and subsequent loading would produce duplicate indicators and orphaned timers. Noiw we dispose this.statusBar with  onClose() before constructing its replacement.

Issue: #2126
…EntriesForLayer().

That controller method immediately starts a replacement restore session; removing the selection afterward closes FeatureInfoView, whose new close() logic clears that replacement session. Also, selection clearing is currently skipped entirely for supported unscoped restore entries (layerId: null), so a hidden feature remains selected and is treated as resolved.

Issue: #2126

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Asynchronous Cesium insertion breaks the promised drawing-layer ordering, while teardown and narrow-screen status-bar issues remain.

Review effort: Balanced
Findings: None

Resolved since last review (8)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Prevent status bar from overflowing narrow mobile viewports

src/​css/​map-view.css:474

The status bar can exceed its containing map on common mobile widths: its maximum width is 18rem + 6.25rem (about 388px), plus the 1rem right offset. On a 320–375px viewport the left side—including the beginning of the loading message—is off-screen. Cap the box to the available width (or add an equivalent responsive rule).

Medium severity Stop loading tracking for all assets during view teardown

src/​js/​views/​maps/​CesiumWidgetView.js:1720

Tracking started here is stopped only when an individual asset is removed. Normal CesiumWidgetView.onClose() destroys the widget without calling stopLoadingStateTracking() on the remaining layers, leaving tileset event callbacks and the imagery provider's patched requestImage retained by the map models after route teardown. Ensure view teardown stops tracking for every attached asset before destroying the widget.

Medium severity Raise drawing sources after asynchronous collection insertion

src/​js/​views/​maps/​CesiumWidgetView.js:1753

DataSourceCollection.add() in the bundled Cesium build resolves the insertion asynchronously, so this immediate raise runs before the newly added source is in the collection. When a normal vector layer is added after the drawing source, the drawing source is raised first and then the normal layer is appended above it, leaving the polygon buried. Raise the flagged sources only after the add promise resolves.

…drawing layer is effectively raised after async collection insertion

CesiumWidgetView.onClose() now calls stopLoadingStateTracking() before destroying the widget.

DataSourceCollection.add() in the bundled Cesium build resolves the insertion asynchronously, so the previously implemented immediate raise ran before the newly added source was in the collection. When a normal vector layer was added after the drawing source, the drawing source was raised first and then the normal layer was appended above it, leaving the polygon buried. Now we raise the flagged sources only after the add promise resolves.

Issue: #2126
@DarianGill
DarianGill requested a review from robyngit September 23, 2026 04:14
@DarianGill

Copy link
Copy Markdown
Contributor Author

Ok, this became quite the behemoth, but I think it's in a robust and functional spot ready for your review now @robyngit :) Don't hesitate to lemme know if that's not the case, or if there are other obvious improvements I should make to make our lives easier in future.

Invalid JSDoc type syntax: {featureId: string, layerId: (string|null)}[] isn't parseable by JSDoc's type parser — it doesn't support appending [] to an inline object-literal type. Changed all 11 occurrences (in SearchParams.js, ActiveFeatureRestoreController.js, Map.js) to Array<{featureId: string, layerId: (string|null)}>.

Template crash: In publish.js:372, buildMemberNav unconditionally added an "Other" category to the nav even when no doclets fell into it, then called .forEach on the missing bucket. Guarded with (organizedItems[category] || []). This bug was previously masked because the type-parse errors caused some doclets to default into the "Other" category.

Issue: #2126

@robyngit robyngit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is so well done, @DarianGill. You call this PR a "behemoth" but actually, it was a pleasure to review. Very readable and structured approach, and you thought of all the details. More importantly, the indicator works so well! I already hate using the prod PDG portal that doesn't have it yet. The expansion/collapse behaviour is smooth, and it now draws the appropriate amount of attention (not too flashy or in the way). It's small in terms of screen real estate, but huge in the impact it will have on making the map just so much more usable. Thanks!!

Added some comments, questions, suggestions, but none are merge blocking.

Comment thread src/js/models/maps/LayerLoadingCoordinator.js
Comment thread src/js/models/maps/Map.js Outdated
Comment thread src/js/views/maps/LayerLoadingIndicatorView.js
Comment thread src/js/views/maps/LayerLoadingIndicatorView.js Outdated
Comment thread src/js/views/maps/LayerLoadingIndicatorView.js Outdated
Comment thread src/js/models/maps/assets/CesiumImagery.js
@@ -38,7 +38,7 @@ define([
this.renderLegendContainer();

if (this.model.get("showScaleBar")) {
this.renderScaleBar();
this.renderStatusBar();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

showScaleBar means show/hide the scale bar, but this now also blocks rendering the loading indicator.

Could we render the loading row independently of the scale bar? (and if needed in the future, add a mapConfig option for showLoadingIndicator)?

To do that, we'd need to remove the condition here and move it to MapStatusBarView, and make it only block the rendering of the scaleBarView.

If it's too much surgery for this PR, we can always address this later. Not a merge blocker.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@robyngit Yeah, that's the most configurable approach, but wouldn't the case where the scale bar isn't visible and the loading indicator is, put us back at the noisy popping in and out of view ux we moved to the scale bar to avoid? I think I'll leave this for a future issue. I could totally get behind a semantic clarification of showStatusBar for the mapConfig option, but I'm not sure we want the view to have just the loading indicator.

Comment thread src/js/models/maps/assets/Cesium3DTileset.js
Comment thread src/js/models/maps/LayerLoadingCoordinator.js Outdated
Comment thread src/js/models/maps/ActiveFeatureRestoreController.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Indicate that a layer is updating/loading in Cesium

3 participants