-
Notifications
You must be signed in to change notification settings - Fork 64
Embed side panels in map view #784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Embed side panels in map view #784
Conversation
|
Integration tests report: appsharing.space |
|
Dang, Guy, that was quick! Nice job! (I'm working on reviewing your other PR right now 😸 ) Is this ready for a full review? If not, can you please convert to draft? I think to make this releasable we need these panels to be completely closable, and perhaps off-by-default. It'd be nice to come up with a descriptive name for each panel. For the left panel (layers and filters), I think we can call it the "Layers menu" and use a "layers" icon for the button to open it . The right panel is a bit harder. It includes object (layer & source) properties, annotations, and the identify results. What could we call that? "Details menu"? A "cog" icon? Or a "list" icon? What do you all think? I like the idea of completely removing the existing side panel icons so we can have a dedicated menu for each instance of JupyterGIS, as the user could have many open (widget, sidepanel, or project file) and it can be confusing when the side panel only shows information for one at a time. Nice :) |
It isn't ready for a review, I have converted it to a draft request, I don't really have any thoughts for the names of the panels. |
|
Happy to be on the same page @mfisher87 ! I really like the cleanup that this PR involves. No need to mess around with document trackers and all anymore. Concerning the UX/UI, I had the following in mind (I'll also share ideas by @trungleduc here)
Note: we may want to explore wrapping those in an OpenLayers control component, that way it's more integrated into the map, does not conflict with other controls (zoom in/out), and would be visible when making the map fullscreen using the fullscreen control Taking inspiration from Felt's UI really, I feel like they've done things right: Screencast.from.2024-06-20.09-05-25.mp4 |
packages/base/src/Lumino.tsx
Outdated
| } | ||
| }, [ref, children]); | ||
| return ( | ||
| <div id={id} ref={ref} style={{ height: height, minHeight: height }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <div id={id} ref={ref} style={{ height: height, minHeight: height }} /> | |
| <div ref={ref} style={{ height: height, minHeight: height }} /> |
packages/base/src/Lumino.tsx
Outdated
| }; | ||
|
|
||
| Lumino.defaultProps = { | ||
| id: 'lumino-id', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use an id for that component, ids are supposed to be unique in the page and it looks like this code will most eventually make duplicates.
| id: 'lumino-id', |
3c2f5ea to
dd46462
Compare
💯 💯 💯 One thing I think we should watch out for is that some updates may not be visible to the user anymore. For example the "Annotations" tab might be deselected most of the time. When the annotations list changes, it would be really nice to present an indicator that this tab has new information, e.g. like GitHub presents a blue circle on the notifications button when there are new notifications. When using the "identify" tool, I think we should just auto-open that tab when the user identifies a feature; they always want to see the result of that operation (or they wouldn't be doing it ;) ). |
|
Agreed 100% with everything you said. We were having a pair programming session with @Gauss-Taylor-Euler yesterday, we'll move all this code to pure react components along the way, no more lumino |
4c05acb to
b891b9b
Compare
Demo.panels.mp4 |
0265dc1 to
081f74d
Compare
martinRenou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I made some more suggestions
No problem I am looking into it. |
demo_panel_embed.mp4 |
c67caa6 to
94cf081
Compare
I think adding shadow effect to the panels can make it look better. |
shadow-demo.mp4 |
martinRenou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more things I noticed while testing:
The panels should take 80% of the available height and we should be able to scroll through the tabs content if it goes over that available space:
See how we can't scroll through identified features if there are too many (it goes out of screen)
The temporal component is now visible anymore (hidden by the panels)
For now we could probably live with setting z-index: 99 on the temporal component so that it goes on top of the panels, but in a later PR we should make something nicer (maybe the temporal component should be yet another tab)
|
Beautiful 🤩
Today in the sync meeting, Martin and I discussed that perhaps the current "Identify" panel could be an "Active tool" panel; depending on which tool the user selects, the UI for that tool can go there. E.g. if "identify" mode is active, it works as it does now; if "time controller" is active, it shows the time controller UI. (To be clear: not for this PR; created #827) |
1aa6500 to
daaaaee
Compare
|
bot please update snapshots!!! |
|
Hey Guy, just wanted to double-check you noticed some of my review comments are hidden by GitHub under "N hidden conversations"? |
|
Yes I looked into them , normally all of them are resolved. |
Hey Guy, one missed suggestion hidden by the GitHub UI. You can see the hidden suggestions by clicking "Load more..." above: It can help to keep track of them if you mark them as resolved as you go! |
297761d to
db109ba
Compare
736b3ba to
8d65803
Compare
|
bot please update snapshots!! |
|
Triggering CI |
mfisher87
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, awesome work!
|
bot please update snapshots!! |
|
Triggering CI |
|
Let's goooo! Thanks a ton @Gauss-Taylor-Euler for this, and thanks to @mfisher87 for the in-depth reviews ❤️ |
|
Great Success everyone 🎉 🚀 |
|
Awesome work @Gauss-Taylor-Euler !! |
Embedded panels
Embedded side panels, resolves #824
Demo :
demo_panel_embed.mp4
Checklist
Resolves #XXX.📚 Documentation preview: https://jupytergis--784.org.readthedocs.build/en/784/
💡 JupyterLite preview: https://jupytergis--784.org.readthedocs.build/en/784/lite