Conversation
|
|
|
Integration tests report: appsharing.space |
|
Preview using JupyterLite: appsharing.space |
|
Docs preview: appsharing.space |
|
Sick!!! |
gjmooney
force-pushed
the
identify_tool
branch
3 times, most recently
from
abf2261 to
5247bef
Compare
|
@gjmooney ready for the WIP indicator be removed from the PR title? |
mfisher87
left a comment
mfisher87
left a comment
There was a problem hiding this comment.
Looks great to me! I don't yet feel confident enough in the overall architecture to "approve" such a big change, so I'll just leave my comments :)
| fill: new Fill({ | ||
| color: '#f37626' | ||
| }) |
There was a problem hiding this comment.
I think this looks great! But I'm also wondering if we should more closely mirror the QGIS behavior which is to apply a heavy red stroke to identified features. Will Jupyter Orange more readily conflict with common colormaps? In QGreenland we chose a white-to-orange colormap for earthquakes, I don't really remember why, but for that colormap, Jupyter Orange would not stand out.
There was a problem hiding this comment.
Personally, I think the red QGIS uses is hideous, and hard to see. Any color we use for the highlight could potentially conflict with a colormap, ideally the highlight color should be user customizable (along with the width, hit tolerance, etc.). For now I made the color a little more red and added a border so hopefully they stand out a bit more.
There was a problem hiding this comment.
I agree the color in QGIS is pretty ugly :) But since the effect in QGIS is a heavy stroke on the existing point, even if the color conflicts, the feature becomes visibly larger. We can always continue to tweak on user feedback; thanks for hearing me out!
| label: '', | ||
| commands: options.commands | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Thinking aloud about the future, we could have more buttons here for "point identify", "polygon identify", "bbox identify", and "normal mode"?
| syncViewport(viewport?: IViewPortState, emitter?: string): void { | ||
| this.sharedModel.awareness.setLocalStateField('viewportState', { | ||
| value: viewport, | ||
| emitter: emitter |
There was a problem hiding this comment.
Let's ban this style with a lint rule? :)
There was a problem hiding this comment.
🤷 I'm fine with either way
This is super slick!! 🤩 |
martinRenou
left a comment
martinRenou
left a comment
There was a problem hiding this comment.
Amazing! That looks neat 😄
As a follow-up, it may be nice to make the list of selected features on the right more interactive (highlight/pan-to feature on click etc). I will open an issue to track this.
|
please update snapshots! |
There was a problem hiding this comment.
Could this be an actual issue with the PR?
There was a problem hiding this comment.
I removed the stroke for the default style for some reason? I'll just put it back.
gjmooney
force-pushed
the
identify_tool
branch
from
68b5015 to
b83bfbc
Compare
* Identify tool start * Collapsable attempt * Keep UI basic * Handle HTML * Add toolbar icon * Add identify toggle * Move filters to left panel * Set up identify panel * Move features list to panel * Grid attempt * Get panel looking good * Improve CSS * Clear panel on file change * Get toolbar to work with multiple files * Clean up * Add identify for webgltile layers * Handle no selected layer * Highlight selected vector features * Use getFeatures for interaction * Use feature ref in handler * Display collaborators features if following * Update identify switch * Remove unused execute args * Make identify button toggleable and add esc keybinding * Tweak highlight color * Require layer to be selected * Have first feature expanded by default * Iterate on layer requirement * Put stroke back * Update Playwright Snapshots --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
3 participants
First iteration of identify tool mentioned in #237
This only works for tiff and vector layers right now.
You can also see the identified features of the user you are following now
