widgets for tilesets example #32
Conversation
There was a problem hiding this comment.
Looks good! A minor comment only
Other than that, here's a TO-DO for me before approving/merging
- Add a comment explaining
getDroppingPercent - Improve copies, including readme
- Update caption
- Use the correct token (potentially I'll have to move the tileset to demo-tilesets)
widgets-tileset/index.ts
Outdated
| import './style.css'; | ||
| import maplibregl from 'maplibre-gl'; | ||
| import 'maplibre-gl/dist/maplibre-gl.css'; | ||
| import {WebMercatorViewport, Deck, Color} from '@deck.gl/core'; |
There was a problem hiding this comment.
You aren't really using Color no? @juandjara
There was a problem hiding this comment.
No. I installed prettier as per @donmccurdy suggestion. Made it explicit by adding a script to the package.json of this example so now we can format this code with yarn run format. It will detect inconsistencies like this and others.
|
Approved so I'm not blocked later but please don't merge yet 🙏 |
widgets-tileset/index.ts
Outdated
| droppingWarningSmall.classList.add('hidden'); | ||
| droppingWarningBig.classList.add('hidden'); |
There was a problem hiding this comment.
Could we instead show/hide these messages with...
el.style.display = 'none';
el.style.display = '';Just in the interest of showing best practice , ideally they'd be shown/hidden from accessibility tools like screenreaders as well as visually.
There was a problem hiding this comment.
I built this way to have the elements initially hidden before the JS loads.
I could use this other behavior and add the initial display: none to the widgets class
There was a problem hiding this comment.
Some inconsistent formatting like semicolons in the file, let's just be sure to run prettier before merging!
There was a problem hiding this comment.
Ok, I added a format script to the package.json of this example to not forget to run prettier on this.
widgets-tileset/index.ts
Outdated
| dataSource.widgetSource.loadTiles(tiles) | ||
| if (!tilesLoaded) { | ||
| tilesLoaded = true; | ||
| renderWidgets(); |
There was a problem hiding this comment.
Very minor and doesn't necessarily need to be changed, but would it be better to call something debounced like debouncedUpdateSpatialFilter here? I don't expect the onViewportLoad callback to run that often, but it might trigger just before/after a viewstate change, and so we'd end up doing the calculations twice since they don't share a debounce.
Would matter a bit more for tilesets with many features, or for rasters.
I've done something similar in:
There was a problem hiding this comment.
Done.
This came from a bug where onViewStateChange was being called before onViewportLoad was called for the first time, so the app was trying to do an extractFeatures before having called loadTiles, so I made sure to set a boolean for a loading state there and to call extractFeatures also depending on that boolean in onViewportLoad
Co-authored-by: Don McCurdy <[email protected]>
…B/deck.gl-examples into feature/sc-461330/widget-tilesets
No description provided.