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

Annulus draw tool #356

Merged
merged 2 commits into from
Jun 16, 2023
Merged

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jun 6, 2023

Description

We need this for Imviz annulus support. This blocks:

cc @dhomeier and @astrofrog

@pllim
Copy link
Contributor Author

pllim commented Jun 6, 2023

p.s. I am not good with the icon stuff, so if you have someone over at Glueviz that makes the proper icons, please feel free to replace the file directly on my branch or open a PR to this PR. Thanks!

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Patch coverage: 31.37% and project coverage change: -0.57 ⚠️

Comparison is base (8ec97fb) 89.96% compared to head (5149e4e) 89.40%.

❗ Current head 5149e4e differs from pull request most recent head f0fd367. Consider uploading reports for the commit f0fd367 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #356      +/-   ##
==========================================
- Coverage   89.96%   89.40%   -0.57%     
==========================================
  Files          86       86              
  Lines        4523     4567      +44     
==========================================
+ Hits         4069     4083      +14     
- Misses        454      484      +30     
Impacted Files Coverage Δ
glue_jupyter/bqplot/common/tools.py 65.84% <31.37%> (-5.33%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

For some reason the icon looks strange for me when using it in the regular scatter viewer (I edited the tools to add this one in to test it out):

Screenshot from 2023-06-15 14-50-46

Do you see that too or does it work ok on your side?

UI-wise, I wonder whether after the first click to select, the user should be able to then continue moving the cursor to edit the inner radius? We don't have to add it here if it's a blocker since this tool isn't enabled by default but just something to think about for future and which might be helpful in jdaviz too.

@pllim
Copy link
Contributor Author

pllim commented Jun 15, 2023

icon looks strange

I am not surprised if it looks strange. I don't have time or interest to make pretty icon especially since Jdaviz uses its own icons anyway and we have a professional designer doing that for us. So, if you have a prettier version, please replace the one in this PR.

I wonder whether after the first click to select, the user should be able to then continue moving the cursor to edit the inner radius?

That could be a follow-up PR. Would be nice to be able to do that at draw time, yes, but I am just not sure what kind of click and when would be the most intuitive, so definitely need some iterations.

@astrofrog
Copy link
Member

I have fixed the icon:

Screenshot 2023-06-16 at 12 07 45

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Thanks!

@astrofrog astrofrog merged commit 2f2b383 into glue-viz:main Jun 16, 2023
@astrofrog astrofrog added the enhancement New feature or request label Jun 16, 2023
@astrofrog astrofrog changed the title FEAT: Annulus draw tool Annulus draw tool Jun 16, 2023
@pllim pllim deleted the annulus-eclipse-of-my-heart branch June 16, 2023 21:21
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