Implemented Dark Mode and Pop Up notification #482
Conversation
WalkthroughThis PR introduces dark mode support and node click popup functionality to NetJSONGraph. Dark mode configuration includes an "auto" detection setting, dark tile URLs for map rendering, and CSS styling for UI elements. When enabled, theme synchronization is established via DOM observers and media query listeners for automatic theme updates. Node popups are triggered on click events and render via Leaflet when configured. Five new public methods handle theme detection, tile updates, and popup display. Sequence Diagram(s)sequenceDiagram
participant User
participant App as onLoad()
participant Theme as applyTheme()
participant DOM as Document
participant MQL as Media Query Listener
participant Obs as Mutation Observer
User->>App: Load NetJSONGraph
App->>Theme: Call applyTheme()
Theme->>DOM: Apply dark mode class
Theme->>DOM: Update Leaflet tiles
alt darkMode === "auto"
Note over App: Install observers
App->>MQL: Listen to prefers-color-scheme
App->>Obs: Watch documentElement<br/>attributes
end
par OS/Theme Change Detection
MQL-->>Theme: prefers-color-scheme changed
Obs-->>Theme: data-theme/class changed
end
Theme->>DOM: Re-apply theme on change
Theme->>DOM: Update tiles for new theme
sequenceDiagram
participant User as User/Browser
participant Handler as onClickElement()
participant Popup as showNodePopup()
participant Leaflet as Leaflet
participant DOM as Map Container
User->>Handler: Click node
Handler->>Handler: Check nodePopupOnClick
Handler->>Popup: showNodePopup(node, nodeInfo)
Popup->>Popup: getPopupLatLng(node)
Popup->>Leaflet: L.popup with content
Leaflet->>Popup: Return popup instance
Popup->>Leaflet: .openOn(map)
Leaflet->>DOM: Render popup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements dark mode support with automatic theme detection and a node popup feature for map visualizations in the NetJSONGraph library.
Key Changes:
- Dark mode with three modes: forced on (
true), forced off (false), or automatic ("auto") that responds to document theme attributes and OS preferences - Node click popup functionality for map and indoor map renders, displaying node information anchored to geographic coordinates
- Dynamic tile layer switching between light and dark map tiles when theme changes
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/js/netjsongraph.render.js |
Adds theme detection (isDarkModeActive), tile layer swapping (updateLeafletTilesForTheme), theme application (applyTheme), coordinate resolution (getPopupLatLng), and popup display (showNodePopup) methods to the render class |
src/js/netjsongraph.js |
Implements theme initialization and sets up MutationObserver and media query listeners to track theme changes when darkMode is set to "auto" |
src/js/netjsongraph.config.js |
Adds configuration options for dark mode behavior, dark tile layers, node popup behavior, and integrates popup display into the node click handler |
src/css/netjsongraph-theme.css |
Defines dark mode styles for UI components including sidebar, tooltips, map controls, and node popups using the .njg-dark-mode class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Determine popup content | ||
| let content = null; | ||
| if (typeof this.config.nodePopupContent === "function") { | ||
| content = this.config.nodePopupContent.call(this, node, nodeInfo, this); | ||
| } | ||
|
|
||
| if (content === undefined || content === null || content === "") { | ||
| const title = | ||
| node?.label || | ||
| node?.name || | ||
| node?.properties?.name || | ||
| (node?.id !== undefined && node?.id !== null ? String(node.id) : "Node"); | ||
| content = `<div class="njg-node-popup-content"><strong>${title}</strong></div>`; | ||
| } | ||
|
|
||
| const popupOpts = this.config.nodePopupOptions || {}; | ||
| const popup = L.popup(popupOpts).setLatLng(latLng).setContent(content); | ||
| popup.openOn(this.leaflet); |
There was a problem hiding this comment.
Potential XSS vulnerability: The popup content from user-provided callback (this.config.nodePopupContent) or from node data (node.label, node.name, etc.) is directly passed to Leaflet's setContent() without sanitization. If node data comes from an untrusted source, this could allow script injection. Consider sanitizing the content or documenting that users must sanitize content in their nodePopupContent callback.
| '© <a href="https://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors, ' | ||
| + 'tiles by <a href="https://carto.com/">CARTO</a>', |
There was a problem hiding this comment.
The concatenation operator on line 271 uses a plus sign at the start of the next line, which is inconsistent with the concatenation on line 270 where the plus is at the end of the line. For consistency and better readability, move the plus operator to the end of line 270 rather than the beginning of line 271.
| '© <a href="https://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors, ' | |
| + 'tiles by <a href="https://carto.com/">CARTO</a>', | |
| '© <a href="https://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors, ' + | |
| 'tiles by <a href="https://carto.com/">CARTO</a>', |
| // Theme support (dark mode): keep map tiles + info UI consistent. | ||
| // Applies once on load, and (when darkMode is "auto") tracks document theme changes. | ||
| if (this.utils && typeof this.utils.applyTheme === "function") { | ||
| this.utils.applyTheme.call(this); | ||
|
|
||
| if (this.config && this.config.darkMode === "auto" && !this._njgThemeObserver) { | ||
| const apply = () => this.utils.applyTheme.call(this); | ||
|
|
||
| // Track html class / data-theme changes (common theme toggles) | ||
| try { | ||
| const docEl = document.documentElement; | ||
| const observer = new MutationObserver(() => apply()); | ||
| observer.observe(docEl, {attributes: true, attributeFilter: ["class", "data-theme"]}); | ||
| this._njgThemeObserver = observer; | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
|
|
||
| // Track OS theme changes | ||
| try { | ||
| const mql = window.matchMedia("(prefers-color-scheme: dark)"); | ||
| const handler = () => apply(); | ||
| if (mql && typeof mql.addEventListener === "function") { | ||
| mql.addEventListener("change", handler); | ||
| this._njgThemeMql = {mql, handler}; | ||
| } else if (mql && typeof mql.addListener === "function") { | ||
| mql.addListener(handler); | ||
| this._njgThemeMql = {mql, handler}; | ||
| } | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The theme initialization code lacks test coverage. Since the repository has comprehensive automated testing (test/netjsongraph.dom.test.js, test/netjsongraph.spec.js), the new theme observer setup, MutationObserver configuration, and media query listener registration should have test cases to verify they work correctly and handle edge cases (e.g., when darkMode is "auto" vs boolean values).
| if (this.config && this.config.darkMode === "auto" && !this._njgThemeObserver) { | ||
| const apply = () => this.utils.applyTheme.call(this); | ||
|
|
||
| // Track html class / data-theme changes (common theme toggles) | ||
| try { | ||
| const docEl = document.documentElement; | ||
| const observer = new MutationObserver(() => apply()); | ||
| observer.observe(docEl, {attributes: true, attributeFilter: ["class", "data-theme"]}); | ||
| this._njgThemeObserver = observer; | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
|
|
||
| // Track OS theme changes | ||
| try { | ||
| const mql = window.matchMedia("(prefers-color-scheme: dark)"); | ||
| const handler = () => apply(); | ||
| if (mql && typeof mql.addEventListener === "function") { | ||
| mql.addEventListener("change", handler); | ||
| this._njgThemeMql = {mql, handler}; | ||
| } else if (mql && typeof mql.addListener === "function") { | ||
| mql.addListener(handler); | ||
| this._njgThemeMql = {mql, handler}; | ||
| } | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The MutationObserver and media query listeners created for theme tracking are never cleaned up. This can lead to memory leaks if the NetJSONGraph instance is destroyed and recreated multiple times. Consider adding a cleanup method (e.g., destroy() or dispose()) that disconnects the MutationObserver and removes the media query event listeners.
| if (tile.label) { | ||
| if (!baseLayerAdded) { | ||
| tileLayer.addTo(this.leaflet); | ||
| baseLayerAdded = true; | ||
| } | ||
| baseLayers[tile.label] = tileLayer; | ||
| } else { | ||
| tileLayer.addTo(this.leaflet); | ||
| } | ||
| }); | ||
|
|
||
| if (tiles.length > 1) { | ||
| const layerControlOpts = this.config.layerControl || {}; | ||
| this.leaflet._njgBaseLayerControl = L.control | ||
| .layers(baseLayers, {}, layerControlOpts) | ||
| .addTo(this.leaflet); | ||
| } |
There was a problem hiding this comment.
The layer control is only created when there are multiple tiles (tiles.length > 1), but the baseLayers object is always populated for tiles with labels. If only one tile has a label and others don't, the layer control won't be created but layers will still be added to baseLayers. This logic may not handle all tile configurations correctly. Consider whether a layer control should be created when there's at least one labeled tile, or clarify the intended behavior in a comment.
| /** | ||
| * Show a Leaflet popup for a node (map & indoor map). | ||
| * | ||
| * @param {object} node Raw node object passed from click handler | ||
| * @param {object} nodeInfo Optional processed info object (from utils.nodeInfo) | ||
| */ | ||
| showNodePopup(node, nodeInfo = null) { | ||
| if (!this.leaflet) { | ||
| return; | ||
| } | ||
|
|
||
| const latLng = this.utils.getPopupLatLng.call(this, node); | ||
| if (!latLng || Number.isNaN(latLng[0]) || Number.isNaN(latLng[1])) { | ||
| // No coordinates: nothing to anchor the popup to. | ||
| return; | ||
| } | ||
|
|
||
| // Determine popup content | ||
| let content = null; | ||
| if (typeof this.config.nodePopupContent === "function") { | ||
| content = this.config.nodePopupContent.call(this, node, nodeInfo, this); | ||
| } | ||
|
|
||
| if (content === undefined || content === null || content === "") { | ||
| const title = | ||
| node?.label || | ||
| node?.name || | ||
| node?.properties?.name || | ||
| (node?.id !== undefined && node?.id !== null ? String(node.id) : "Node"); | ||
| content = `<div class="njg-node-popup-content"><strong>${title}</strong></div>`; | ||
| } | ||
|
|
||
| const popupOpts = this.config.nodePopupOptions || {}; | ||
| const popup = L.popup(popupOpts).setLatLng(latLng).setContent(content); | ||
| popup.openOn(this.leaflet); | ||
|
|
||
| this._njgCurrentNodePopup = popup; | ||
| } |
There was a problem hiding this comment.
The comment says "Pop Up notification" in the PR title but this implementation is a popup display feature, not a notification system. A "notification" typically refers to toast messages or alerts, while this is a map popup anchored to coordinates. Consider updating documentation to clarify this is a node information popup feature rather than a notification system to avoid confusion.
| /** | ||
| * Determine whether dark mode is currently active. | ||
| * | ||
| * Priority: | ||
| * 1) config.darkMode boolean (force) | ||
| * 2) document theme markers (html.dark-mode or html[data-theme="dark"]) | ||
| * 3) OS preference (prefers-color-scheme) | ||
| */ | ||
| isDarkModeActive() { | ||
| if (typeof this.config.darkMode === "boolean") { | ||
| return this.config.darkMode; | ||
| } | ||
|
|
||
| const docEl = document && document.documentElement; | ||
| if (docEl) { | ||
| if (docEl.classList.contains("dark-mode")) { | ||
| return true; | ||
| } | ||
| const dataTheme = docEl.getAttribute("data-theme"); | ||
| if (typeof dataTheme === "string" && dataTheme.toLowerCase() === "dark") { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| return !!( | ||
| window && | ||
| window.matchMedia && | ||
| window.matchMedia("(prefers-color-scheme: dark)").matches | ||
| ); | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Swap Leaflet tile layers based on the active theme. | ||
| * This only touches base TileLayer instances; overlays (geojson, markers, etc) remain. | ||
| */ | ||
| updateLeafletTilesForTheme(isDark) { | ||
| if (!this.leaflet) { | ||
| return; | ||
| } | ||
|
|
||
| const tiles = Array.isArray(this.config.mapTileConfig) | ||
| ? this.config.mapTileConfig | ||
| : []; | ||
| if (!tiles.length) { | ||
| return; | ||
| } | ||
|
|
||
| // Remove existing tile layers | ||
| Object.keys(this.leaflet._layers || {}).forEach((k) => { | ||
| const layer = this.leaflet._layers[k]; | ||
| if (layer && layer instanceof L.TileLayer) { | ||
| this.leaflet.removeLayer(layer); | ||
| } | ||
| }); | ||
|
|
||
| // Remove existing layer control created by us (if any) | ||
| if (this.leaflet._njgBaseLayerControl) { | ||
| try { | ||
| this.leaflet._njgBaseLayerControl.remove(); | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
| this.leaflet._njgBaseLayerControl = null; | ||
| } | ||
|
|
||
| const baseLayers = {}; | ||
| let baseLayerAdded = false; | ||
|
|
||
| tiles.forEach((tile) => { | ||
| const urlTemplate = | ||
| isDark && tile.darkUrlTemplate ? tile.darkUrlTemplate : tile.urlTemplate; | ||
| const options = | ||
| isDark && tile.darkOptions ? tile.darkOptions : tile.options; | ||
|
|
||
| const tileLayer = L.tileLayer(urlTemplate, options); | ||
| if (tile.label) { | ||
| if (!baseLayerAdded) { | ||
| tileLayer.addTo(this.leaflet); | ||
| baseLayerAdded = true; | ||
| } | ||
| baseLayers[tile.label] = tileLayer; | ||
| } else { | ||
| tileLayer.addTo(this.leaflet); | ||
| } | ||
| }); | ||
|
|
||
| if (tiles.length > 1) { | ||
| const layerControlOpts = this.config.layerControl || {}; | ||
| this.leaflet._njgBaseLayerControl = L.control | ||
| .layers(baseLayers, {}, layerControlOpts) | ||
| .addTo(this.leaflet); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Apply theme to DOM and map tiles. | ||
| */ | ||
| applyTheme() { | ||
| const isDark = this.utils.isDarkModeActive.call(this); | ||
| if (this.el) { | ||
| this.el.classList.toggle("njg-dark-mode", !!isDark); | ||
| } | ||
| // Only maps need tile swapping | ||
| if (this.config && this.config.render === this.utils.mapRender) { | ||
| this.utils.updateLeafletTilesForTheme.call(this, !!isDark); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Resolve popup coordinates from a node-like object. | ||
| * Supports: | ||
| * - NetJSONGraph nodes: node.properties.location or node.location | ||
| * - GeoJSON point array: node.coordinates [lng, lat] | ||
| * - GeoJSON feature geometry: node.geometry.coordinates [lng, lat] | ||
| */ | ||
| getPopupLatLng(node) { | ||
| const loc = node?.properties?.location || node?.location; | ||
| if (loc && typeof loc.lat === "number" && typeof loc.lng === "number") { | ||
| return [loc.lat, loc.lng]; | ||
| } | ||
|
|
||
| if (Array.isArray(node?.coordinates) && node.coordinates.length >= 2) { | ||
| return [node.coordinates[1], node.coordinates[0]]; | ||
| } | ||
|
|
||
| if (Array.isArray(node?.geometry?.coordinates) && node.geometry.coordinates.length >= 2) { | ||
| return [node.geometry.coordinates[1], node.geometry.coordinates[0]]; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Show a Leaflet popup for a node (map & indoor map). | ||
| * | ||
| * @param {object} node Raw node object passed from click handler | ||
| * @param {object} nodeInfo Optional processed info object (from utils.nodeInfo) | ||
| */ | ||
| showNodePopup(node, nodeInfo = null) { | ||
| if (!this.leaflet) { | ||
| return; | ||
| } | ||
|
|
||
| const latLng = this.utils.getPopupLatLng.call(this, node); | ||
| if (!latLng || Number.isNaN(latLng[0]) || Number.isNaN(latLng[1])) { | ||
| // No coordinates: nothing to anchor the popup to. | ||
| return; | ||
| } | ||
|
|
||
| // Determine popup content | ||
| let content = null; | ||
| if (typeof this.config.nodePopupContent === "function") { | ||
| content = this.config.nodePopupContent.call(this, node, nodeInfo, this); | ||
| } | ||
|
|
||
| if (content === undefined || content === null || content === "") { | ||
| const title = | ||
| node?.label || | ||
| node?.name || | ||
| node?.properties?.name || | ||
| (node?.id !== undefined && node?.id !== null ? String(node.id) : "Node"); | ||
| content = `<div class="njg-node-popup-content"><strong>${title}</strong></div>`; | ||
| } | ||
|
|
||
| const popupOpts = this.config.nodePopupOptions || {}; | ||
| const popup = L.popup(popupOpts).setLatLng(latLng).setContent(content); | ||
| popup.openOn(this.leaflet); | ||
|
|
||
| this._njgCurrentNodePopup = popup; | ||
| } |
There was a problem hiding this comment.
The new dark mode and popup features lack test coverage. No tests are added for isDarkModeActive(), updateLeafletTilesForTheme(), applyTheme(), getPopupLatLng(), or showNodePopup() methods. Since the repository has comprehensive test files (e.g., test/netjsongraph.render.test.js), these new features should have corresponding test cases to verify the dark mode detection logic, theme switching, and popup functionality work correctly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @src/css/netjsongraph-theme.css:
- Around line 79-130: Prettier formatting errors are present in the CSS block
for dark mode selectors (e.g., .njg-container.njg-dark-mode,
.njg-container.njg-dark-mode .njg-sideBar, .njg-container.njg-dark-mode
.leaflet-popup.njg-node-popup); fix by running Prettier on the file (npx
prettier --write src/css/netjsongraph-theme.css) or reformat the file to match
Prettier rules (consistent spacing, semicolons, braces, and final newline) so
the pipeline passes.
In @src/js/netjsongraph.js:
- Around line 156-189: Rename the internal observer properties to remove leading
underscores (e.g., use njgThemeObserver and njgThemeMql instead of
_njgThemeObserver/_njgThemeMql) to satisfy ESLint no-underscore-dangle, and
update the guard/installation logic in the theme setup so each observer is
checked and created independently (check njgThemeObserver before creating the
MutationObserver and check njgThemeMql before adding the media query listener)
to avoid double-registration; ensure you also set the new property names when
assigning the observer and cleanup code uses the new names.
In @src/js/netjsongraph.render.js:
- Around line 141-163: The getPopupLatLng function uses optional chaining (e.g.,
node?.properties?.location) which causes parse errors in environments without
that syntax; replace all optional chaining in getPopupLatLng with explicit
null/undefined checks (e.g., if (node && node.properties &&
node.properties.location) { const loc = node.properties.location; } and similar
guards for node.location, node.coordinates and node.geometry.coordinates) so the
logic and return values remain the same but without using the ?. operator.
🧹 Nitpick comments (2)
src/js/netjsongraph.render.js (2)
64-125: Consider using Leaflet's public API instead of accessing private properties.Line 81 accesses
this.leaflet._layers, which is a private Leaflet implementation detail. This could break in future Leaflet versions.🔎 Safer alternative using public API
- // Remove existing tile layers - Object.keys(this.leaflet._layers || {}).forEach((k) => { - const layer = this.leaflet._layers[k]; - if (layer && layer instanceof L.TileLayer) { - this.leaflet.removeLayer(layer); - } - }); + // Remove existing tile layers using public API + this.leaflet.eachLayer((layer) => { + if (layer instanceof L.TileLayer) { + this.leaflet.removeLayer(layer); + } + });Note:
_njgBaseLayerControlis your own custom property, so that's fine.
165-202: Fix optional chaining and consider closing previous popups.Lines 190-193 use optional chaining which causes the same parsing error as
getPopupLatLng. Additionally, Line 201 stores a reference using_njgCurrentNodePopupwhich may trigger ESLint warnings.🔎 Refactor to remove optional chaining and improve popup management
showNodePopup(node, nodeInfo = null) { if (!this.leaflet) { return; } const latLng = this.utils.getPopupLatLng.call(this, node); if (!latLng || Number.isNaN(latLng[0]) || Number.isNaN(latLng[1])) { // No coordinates: nothing to anchor the popup to. return; } + // Close any existing node popup before opening a new one + if (this.njgCurrentNodePopup) { + this.leaflet.closePopup(this.njgCurrentNodePopup); + } + // Determine popup content let content = null; if (typeof this.config.nodePopupContent === "function") { content = this.config.nodePopupContent.call(this, node, nodeInfo, this); } if (content === undefined || content === null || content === "") { const title = - node?.label || - node?.name || - node?.properties?.name || - (node?.id !== undefined && node?.id !== null ? String(node.id) : "Node"); + (node && node.label) || + (node && node.name) || + (node && node.properties && node.properties.name) || + (node && node.id !== undefined && node.id !== null ? String(node.id) : "Node"); content = `<div class="njg-node-popup-content"><strong>${title}</strong></div>`; } const popupOpts = this.config.nodePopupOptions || {}; const popup = L.popup(popupOpts).setLatLng(latLng).setContent(content); popup.openOn(this.leaflet); - this._njgCurrentNodePopup = popup; + this.njgCurrentNodePopup = popup; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
src/css/netjsongraph-theme.csssrc/js/netjsongraph.config.jssrc/js/netjsongraph.jssrc/js/netjsongraph.render.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/netjsongraph.render.js (1)
examples/load_data_geo_extent/index.js (1)
node(406-413)
🪛 GitHub Actions: netjsongraph.js CI BUILD
src/css/netjsongraph-theme.css
[warning] 1-1: Code style issues found. Run Prettier with --write to fix code style issues in this file.
src/js/netjsongraph.render.js
[error] 149-149: eslint: Parsing error: Unexpected token .
src/js/netjsongraph.js
[error] 161-161: eslint: Unexpected dangling '_' in '_njgThemeObserver' (no-underscore-dangle)
[error] 169-169: eslint: Unexpected dangling '_' in '_njgThemeObserver' (no-underscore-dangle)
[error] 180-180: eslint: Unexpected dangling '_' in '_njgThemeMql' (no-underscore-dangle)
[error] 183-183: eslint: Unexpected dangling '_' in '_njgThemeMql' (no-underscore-dangle)
src/js/netjsongraph.config.js
[warning] 1-1: Code style issues found. Run Prettier with --write to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (6)
src/js/netjsongraph.render.js (2)
29-62: LGTM: Dark mode detection with proper fallbacks.The priority-based detection (config → document → OS) is well-structured, and the try-catch appropriately handles environments without
matchMediasupport.
127-139: LGTM: Clean theme application logic.The conditional tile update for map mode is appropriate, and the DOM class toggle is straightforward.
src/js/netjsongraph.config.js (4)
47-51: LGTM: Well-documented dark mode configuration.The "auto" default and clear comment explaining the three modes (true/false/"auto") make this configuration intuitive.
305-316: LGTM: Backward-compatible popup configuration.Disabling popups by default (
nodePopupOnClick: false) preserves existing behavior while allowing opt-in customization.
375-385: LGTM: Well-guarded popup invocation.The conditional popup display properly checks for all required dependencies (
type === "node",config.nodePopupOnClick,this.leaflet, and function existence) before callingshowNodePopup.
263-272: CARTO basemaps do not require authentication and no Prettier formatting issues are evident.The CARTO basemaps service (basemaps.cartocdn.com) is publicly accessible and does not require API keys or authentication for production use. The tile URL structure is correct and will work without any credentials.
Regarding Prettier formatting: the code appears properly formatted. If there are actual pipeline issues, please share the specific error message, but the string concatenation style shown is valid.
| /* Dark mode (applied by JS via .njg-dark-mode) */ | ||
| .njg-container.njg-dark-mode { | ||
| color: #e0e0e0; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .njg-sideBar { | ||
| background-color: #1e1e1e; | ||
| color: #e0e0e0; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .njg-metaData, | ||
| .njg-container.njg-dark-mode .njg-infoContainer { | ||
| color: #e0e0e0; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .njg-tooltip { | ||
| background: #1e1e1e !important; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .njg-tooltip-key, | ||
| .njg-container.njg-dark-mode .njg-tooltip-value { | ||
| color: #e0e0e0; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .leaflet-popup-content-wrapper, | ||
| .njg-container.njg-dark-mode .leaflet-popup-tip { | ||
| background: #1e1e1e; | ||
| color: #e0e0e0; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .leaflet-control-attribution { | ||
| background: rgba(30, 30, 30, 0.8); | ||
| color: #e0e0e0; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .leaflet-control-attribution a { | ||
| color: #e0e0e0; | ||
| } | ||
|
|
||
| /* Node click popup */ | ||
| .njg-container .leaflet-popup.njg-node-popup .leaflet-popup-content-wrapper, | ||
| .njg-container .leaflet-popup.njg-node-popup .leaflet-popup-tip { | ||
| background: #fff; | ||
| color: #000; | ||
| } | ||
|
|
||
| .njg-container.njg-dark-mode .leaflet-popup.njg-node-popup .leaflet-popup-content-wrapper, | ||
| .njg-container.njg-dark-mode .leaflet-popup.njg-node-popup .leaflet-popup-tip { | ||
| background: #1e1e1e; | ||
| color: #e0e0e0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix code formatting to pass Prettier checks.
The pipeline is reporting code style issues. Please run Prettier to format this file:
npx prettier --write src/css/netjsongraph-theme.css🤖 Prompt for AI Agents
In @src/css/netjsongraph-theme.css around lines 79 - 130, Prettier formatting
errors are present in the CSS block for dark mode selectors (e.g.,
.njg-container.njg-dark-mode, .njg-container.njg-dark-mode .njg-sideBar,
.njg-container.njg-dark-mode .leaflet-popup.njg-node-popup); fix by running
Prettier on the file (npx prettier --write src/css/netjsongraph-theme.css) or
reformat the file to match Prettier rules (consistent spacing, semicolons,
braces, and final newline) so the pipeline passes.
| // Theme support (dark mode): keep map tiles + info UI consistent. | ||
| // Applies once on load, and (when darkMode is "auto") tracks document theme changes. | ||
| if (this.utils && typeof this.utils.applyTheme === "function") { | ||
| this.utils.applyTheme.call(this); | ||
|
|
||
| if (this.config && this.config.darkMode === "auto" && !this._njgThemeObserver) { | ||
| const apply = () => this.utils.applyTheme.call(this); | ||
|
|
||
| // Track html class / data-theme changes (common theme toggles) | ||
| try { | ||
| const docEl = document.documentElement; | ||
| const observer = new MutationObserver(() => apply()); | ||
| observer.observe(docEl, {attributes: true, attributeFilter: ["class", "data-theme"]}); | ||
| this._njgThemeObserver = observer; | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
|
|
||
| // Track OS theme changes | ||
| try { | ||
| const mql = window.matchMedia("(prefers-color-scheme: dark)"); | ||
| const handler = () => apply(); | ||
| if (mql && typeof mql.addEventListener === "function") { | ||
| mql.addEventListener("change", handler); | ||
| this._njgThemeMql = {mql, handler}; | ||
| } else if (mql && typeof mql.addListener === "function") { | ||
| mql.addListener(handler); | ||
| this._njgThemeMql = {mql, handler}; | ||
| } | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix ESLint violations and improve observer guard logic.
The pipeline is failing due to no-underscore-dangle violations on the property names _njgThemeObserver and _njgThemeMql.
Additionally, Line 161 guards observer installation by checking only !this._njgThemeObserver, but both observers could already exist independently. If _njgThemeMql exists but _njgThemeObserver doesn't, the media query listener would be added twice.
🔎 Proposed fix
Option 1: Remove underscores (recommended)
- if (this.config && this.config.darkMode === "auto" && !this._njgThemeObserver) {
+ if (this.config && this.config.darkMode === "auto" && !this.njgThemeObserver) {
const apply = () => this.utils.applyTheme.call(this);
// Track html class / data-theme changes (common theme toggles)
try {
const docEl = document.documentElement;
const observer = new MutationObserver(() => apply());
observer.observe(docEl, {attributes: true, attributeFilter: ["class", "data-theme"]});
- this._njgThemeObserver = observer;
+ this.njgThemeObserver = observer;
} catch (e) {
// ignore
}
// Track OS theme changes
try {
const mql = window.matchMedia("(prefers-color-scheme: dark)");
const handler = () => apply();
if (mql && typeof mql.addEventListener === "function") {
mql.addEventListener("change", handler);
- this._njgThemeMql = {mql, handler};
+ this.njgThemeMql = {mql, handler};
} else if (mql && typeof mql.addListener === "function") {
mql.addListener(handler);
- this._njgThemeMql = {mql, handler};
+ this.njgThemeMql = {mql, handler};
}
} catch (e) {
// ignore
}
}Option 2: If underscores are necessary, disable ESLint for these lines and improve guard
- if (this.config && this.config.darkMode === "auto" && !this._njgThemeObserver) {
+ // eslint-disable-next-line no-underscore-dangle
+ if (this.config && this.config.darkMode === "auto" && !this._njgThemeObserver && !this._njgThemeMql) {
const apply = () => this.utils.applyTheme.call(this);
// Track html class / data-theme changes (common theme toggles)
try {
const docEl = document.documentElement;
const observer = new MutationObserver(() => apply());
observer.observe(docEl, {attributes: true, attributeFilter: ["class", "data-theme"]});
+ // eslint-disable-next-line no-underscore-dangle
this._njgThemeObserver = observer;
} catch (e) {
// ignore
}
// Track OS theme changes
try {
const mql = window.matchMedia("(prefers-color-scheme: dark)");
const handler = () => apply();
if (mql && typeof mql.addEventListener === "function") {
mql.addEventListener("change", handler);
+ // eslint-disable-next-line no-underscore-dangle
this._njgThemeMql = {mql, handler};
} else if (mql && typeof mql.addListener === "function") {
mql.addListener(handler);
+ // eslint-disable-next-line no-underscore-dangle
this._njgThemeMql = {mql, handler};
}
} catch (e) {
// ignore
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Theme support (dark mode): keep map tiles + info UI consistent. | |
| // Applies once on load, and (when darkMode is "auto") tracks document theme changes. | |
| if (this.utils && typeof this.utils.applyTheme === "function") { | |
| this.utils.applyTheme.call(this); | |
| if (this.config && this.config.darkMode === "auto" && !this._njgThemeObserver) { | |
| const apply = () => this.utils.applyTheme.call(this); | |
| // Track html class / data-theme changes (common theme toggles) | |
| try { | |
| const docEl = document.documentElement; | |
| const observer = new MutationObserver(() => apply()); | |
| observer.observe(docEl, {attributes: true, attributeFilter: ["class", "data-theme"]}); | |
| this._njgThemeObserver = observer; | |
| } catch (e) { | |
| // ignore | |
| } | |
| // Track OS theme changes | |
| try { | |
| const mql = window.matchMedia("(prefers-color-scheme: dark)"); | |
| const handler = () => apply(); | |
| if (mql && typeof mql.addEventListener === "function") { | |
| mql.addEventListener("change", handler); | |
| this._njgThemeMql = {mql, handler}; | |
| } else if (mql && typeof mql.addListener === "function") { | |
| mql.addListener(handler); | |
| this._njgThemeMql = {mql, handler}; | |
| } | |
| } catch (e) { | |
| // ignore | |
| } | |
| } | |
| } | |
| // Theme support (dark mode): keep map tiles + info UI consistent. | |
| // Applies once on load, and (when darkMode is "auto") tracks document theme changes. | |
| if (this.utils && typeof this.utils.applyTheme === "function") { | |
| this.utils.applyTheme.call(this); | |
| if (this.config && this.config.darkMode === "auto" && !this.njgThemeObserver) { | |
| const apply = () => this.utils.applyTheme.call(this); | |
| // Track html class / data-theme changes (common theme toggles) | |
| try { | |
| const docEl = document.documentElement; | |
| const observer = new MutationObserver(() => apply()); | |
| observer.observe(docEl, {attributes: true, attributeFilter: ["class", "data-theme"]}); | |
| this.njgThemeObserver = observer; | |
| } catch (e) { | |
| // ignore | |
| } | |
| // Track OS theme changes | |
| try { | |
| const mql = window.matchMedia("(prefers-color-scheme: dark)"); | |
| const handler = () => apply(); | |
| if (mql && typeof mql.addEventListener === "function") { | |
| mql.addEventListener("change", handler); | |
| this.njgThemeMql = {mql, handler}; | |
| } else if (mql && typeof mql.addListener === "function") { | |
| mql.addListener(handler); | |
| this.njgThemeMql = {mql, handler}; | |
| } | |
| } catch (e) { | |
| // ignore | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: netjsongraph.js CI BUILD
[error] 161-161: eslint: Unexpected dangling '_' in '_njgThemeObserver' (no-underscore-dangle)
[error] 169-169: eslint: Unexpected dangling '_' in '_njgThemeObserver' (no-underscore-dangle)
[error] 180-180: eslint: Unexpected dangling '_' in '_njgThemeMql' (no-underscore-dangle)
[error] 183-183: eslint: Unexpected dangling '_' in '_njgThemeMql' (no-underscore-dangle)
🤖 Prompt for AI Agents
In @src/js/netjsongraph.js around lines 156 - 189, Rename the internal observer
properties to remove leading underscores (e.g., use njgThemeObserver and
njgThemeMql instead of _njgThemeObserver/_njgThemeMql) to satisfy ESLint
no-underscore-dangle, and update the guard/installation logic in the theme setup
so each observer is checked and created independently (check njgThemeObserver
before creating the MutationObserver and check njgThemeMql before adding the
media query listener) to avoid double-registration; ensure you also set the new
property names when assigning the observer and cleanup code uses the new names.
| /** | ||
| * Resolve popup coordinates from a node-like object. | ||
| * Supports: | ||
| * - NetJSONGraph nodes: node.properties.location or node.location | ||
| * - GeoJSON point array: node.coordinates [lng, lat] | ||
| * - GeoJSON feature geometry: node.geometry.coordinates [lng, lat] | ||
| */ | ||
| getPopupLatLng(node) { | ||
| const loc = node?.properties?.location || node?.location; | ||
| if (loc && typeof loc.lat === "number" && typeof loc.lng === "number") { | ||
| return [loc.lat, loc.lng]; | ||
| } | ||
|
|
||
| if (Array.isArray(node?.coordinates) && node.coordinates.length >= 2) { | ||
| return [node.coordinates[1], node.coordinates[0]]; | ||
| } | ||
|
|
||
| if (Array.isArray(node?.geometry?.coordinates) && node.geometry.coordinates.length >= 2) { | ||
| return [node.geometry.coordinates[1], node.geometry.coordinates[0]]; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix parsing error: Optional chaining may not be supported.
The pipeline reports a parsing error at Line 149: "Unexpected token ." This indicates the optional chaining operator (?.) may not be supported in your current JavaScript environment or build configuration.
🔎 Refactor to avoid optional chaining
getPopupLatLng(node) {
- const loc = node?.properties?.location || node?.location;
+ const loc = (node && node.properties && node.properties.location) || (node && node.location);
if (loc && typeof loc.lat === "number" && typeof loc.lng === "number") {
return [loc.lat, loc.lng];
}
- if (Array.isArray(node?.coordinates) && node.coordinates.length >= 2) {
+ if (node && Array.isArray(node.coordinates) && node.coordinates.length >= 2) {
return [node.coordinates[1], node.coordinates[0]];
}
- if (Array.isArray(node?.geometry?.coordinates) && node.geometry.coordinates.length >= 2) {
+ if (
+ node &&
+ node.geometry &&
+ Array.isArray(node.geometry.coordinates) &&
+ node.geometry.coordinates.length >= 2
+ ) {
return [node.geometry.coordinates[1], node.geometry.coordinates[0]];
}
return null;
}Alternatively, ensure your build pipeline includes Babel with the @babel/plugin-proposal-optional-chaining plugin.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Resolve popup coordinates from a node-like object. | |
| * Supports: | |
| * - NetJSONGraph nodes: node.properties.location or node.location | |
| * - GeoJSON point array: node.coordinates [lng, lat] | |
| * - GeoJSON feature geometry: node.geometry.coordinates [lng, lat] | |
| */ | |
| getPopupLatLng(node) { | |
| const loc = node?.properties?.location || node?.location; | |
| if (loc && typeof loc.lat === "number" && typeof loc.lng === "number") { | |
| return [loc.lat, loc.lng]; | |
| } | |
| if (Array.isArray(node?.coordinates) && node.coordinates.length >= 2) { | |
| return [node.coordinates[1], node.coordinates[0]]; | |
| } | |
| if (Array.isArray(node?.geometry?.coordinates) && node.geometry.coordinates.length >= 2) { | |
| return [node.geometry.coordinates[1], node.geometry.coordinates[0]]; | |
| } | |
| return null; | |
| } | |
| getPopupLatLng(node) { | |
| const loc = (node && node.properties && node.properties.location) || (node && node.location); | |
| if (loc && typeof loc.lat === "number" && typeof loc.lng === "number") { | |
| return [loc.lat, loc.lng]; | |
| } | |
| if (node && Array.isArray(node.coordinates) && node.coordinates.length >= 2) { | |
| return [node.coordinates[1], node.coordinates[0]]; | |
| } | |
| if ( | |
| node && | |
| node.geometry && | |
| Array.isArray(node.geometry.coordinates) && | |
| node.geometry.coordinates.length >= 2 | |
| ) { | |
| return [node.geometry.coordinates[1], node.geometry.coordinates[0]]; | |
| } | |
| return null; | |
| } |
🧰 Tools
🪛 GitHub Actions: netjsongraph.js CI BUILD
[error] 149-149: eslint: Parsing error: Unexpected token .
🤖 Prompt for AI Agents
In @src/js/netjsongraph.render.js around lines 141 - 163, The getPopupLatLng
function uses optional chaining (e.g., node?.properties?.location) which causes
parse errors in environments without that syntax; replace all optional chaining
in getPopupLatLng with explicit null/undefined checks (e.g., if (node &&
node.properties && node.properties.location) { const loc =
node.properties.location; } and similar guards for node.location,
node.coordinates and node.geometry.coordinates) so the logic and return values
remain the same but without using the ?. operator.
|
What issue are you addressing with this @vvinit594? |
2 participants
Checklist
Reference to Existing Issue
Closes #.
Please open a new issue if there isn't an existing issue yet.
Description of Changes
Please describe these changes.
Screenshot
Please include any relevant screenshots.