Skip to content
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

Color ramps and classification #177

Merged
merged 40 commits into from
Oct 22, 2024

Conversation

gjmooney
Copy link
Collaborator

@gjmooney gjmooney commented Oct 8, 2024

Implementing #173

I started to add automatic data classification and color ramps. I still need to add other classification methods, add it to other render types, and work on the UI a bit.

colorramp

Copy link
Contributor

github-actions bot commented Oct 8, 2024

Binder 👈 Launch a Binder on branch gjmooney/jupytergis/color_ramps_and_classification

@gjmooney gjmooney added the enhancement New feature or request label Oct 8, 2024
@gjmooney gjmooney force-pushed the color_ramps_and_classification branch 2 times, most recently from 7228014 to 304bcf4 Compare October 16, 2024 10:48
@gjmooney gjmooney marked this pull request as ready for review October 17, 2024 09:19
@martinRenou
Copy link
Member

Just rebased!

Copy link
Contributor

github-actions bot commented Oct 17, 2024

Preview PR at appsharing.space

Copy link
Contributor

github-actions bot commented Oct 17, 2024

Integration tests repot: appsharing.space

@martinRenou
Copy link
Member

martinRenou commented Oct 17, 2024

Looks great!! Tested on the jupyterlite deployment and that works nicely there too

I wondering if it would make sense to have the spinner on top of the entire dialog when fetching and processing the data? With a blurry background blurring the rest of the buttons

Screenshot from 2024-10-17 17-03-30

@gjmooney
Copy link
Collaborator Author

Looks great!! Tested on the jupyterlite deployment and that works nicely there too

I wondering if it would make sense to have the spinner on top of the entire dialog when fetching and processing the data? With a blurry background blurring the rest of the buttons

Yea we could do that. This comment made me realize there's an issue, if you open the symbology panel before the band data is in StateDb it gets stuck loading. Maybe we should keep the band loading in the panel?

@gjmooney gjmooney force-pushed the color_ramps_and_classification branch from ddd24c9 to 65f6d4c Compare October 18, 2024 07:37
@gjmooney gjmooney force-pushed the color_ramps_and_classification branch from 087fee1 to 2dbde22 Compare October 18, 2024 07:44
@martinRenou
Copy link
Member

The colormap names are not visible when using the light theme. I guess we should not use CSS variables for those but harcoded white?

Screenshot from 2024-10-18 15-49-41


Sometimes it stops showing the color ramp that was used for generating the color stops (e.g. when closing the symbology panel and reopening it). I wonder if it would make sense to store the colormap type in the schema so that it's easier for you to restore the state of the symbology panel?

Screenshot from 2024-10-18 15-51-50

@gjmooney
Copy link
Collaborator Author

The colormap names are not visible when using the light theme. I guess we should not use CSS variables for those but harcoded white?

👍

Sometimes it stops showing the color ramp that was used for generating the color stops (e.g. when closing the symbology panel and reopening it). I wonder if it would make sense to store the colormap type in the schema so that it's easier for you to restore the state of the symbology panel?

Yea I was debating whether to save that stuff in the schema instead of StateDb but I didn't want to clutter up the schema too much. I'll look into this a bit and see if I can figure out what's happening before we change the schema.

@gjmooney gjmooney force-pushed the color_ramps_and_classification branch from 9cc9e4c to c025aa1 Compare October 21, 2024 11:28
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks!!!! 🚀

@martinRenou martinRenou merged commit c5fd535 into geojupyter:main Oct 22, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants