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

change cursor according to annotation tool #6132

Merged
merged 24 commits into from Apr 21, 2022

Conversation

Dagobert42
Copy link
Contributor

@Dagobert42 Dagobert42 commented Mar 31, 2022

URL of deployed dev instance (used for testing):

Steps to test:

  • hover on input catchers
  • cycle through active tools

Issues:


@Dagobert42
Copy link
Contributor Author

The lasso and new-bounding-box icons are problematic as they cant be filled without further modification. I replaced the lasso with a crosshair and made new-bounding-box dark but its probably not quite what we want.
Also the transposition of the actual clicking position still needs to be fine-tuned.
Regarding this, would you expect A or B here?

Screenshot 2022-03-31 at 18 42 51

@normanrz
Copy link
Member

normanrz commented Apr 5, 2022

Not sure it is a good idea to replace the built-in cursors, because it makes it harder to know what exact pixel will be targetted.
Also, we need to be careful with copying icons from FontAwesome into our repository. Not sure, if the license permits that.

@Dagobert42
Copy link
Contributor Author

Dagobert42 commented Apr 6, 2022

Not sure it is a good idea to replace the built-in cursors, because it makes it harder to know what exact pixel will be targetted.

Agreed, but I think we found a good compromise with most icons. For now I went through all the SVGs and tried to lign up their click position as best I could.

Regarding the Font Awesome license, I've checked it and it allows for modifications.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome :) Please update the changelog before merging, though!

@Dagobert42 Dagobert42 merged commit a2969d6 into master Apr 21, 2022
@Dagobert42 Dagobert42 deleted the change-cursor-according-to-tool branch April 21, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change cursor according to active tool
4 participants