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

Add pan tool #399

Merged
merged 1 commit into from
Dec 23, 2020
Merged

Conversation

Schweini07
Copy link
Contributor

@luiq54 the second canvas is now dragable.

I also have to learn Git better. I needed one hour for finally putting this up and 1 minute working on it in Godot.

@dphaldes
Copy link
Contributor

Looks great!

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

So, functionality-wise it's very good! It will be a useful tool to have and it works well. However, I am mostly worried about the name of the tool, and its icon. A "move" tool usually refers to a tool that moves the pixels of the image (see #313) and not just the canvas. I think if we renamed it to "Pan" it would be a much better name. A hand icon, like this from Godot
image

also fits better for this purpose. I'd recommend using an icon like that for the Pan tool, and we can keep the one you made for a move tool that moves parts of the image.

If you could make these two changes, and if you agree of course, then I think it's ready to get merged. If you have issues making a hand icon, we could ask @Erevoid to handle that.

(Also, it's best if you avoided including files to the commit that don't have any relevant changes, like Main.tscn)

@Schweini07
Copy link
Contributor Author

Schweini07 commented Dec 22, 2020

Thanks for the feedback! I am getting right at it.
Regarding Main.tscn, I had a lot of problems with Git, but maybe I am able to get it right this time.

@Schweini07
Copy link
Contributor Author

I did the renaming, but I don't think I will get a good hand icon done.
I would have nothing against @Erevoid making those.
Should I already push, with the old icons, or wait?

@OverloadedOrama
Copy link
Member

I did the renaming, but I don't think I will get a good hand icon done.
I would have nothing against @Erevoid making those.
Should I already push, with the old icons, or wait?

Go ahead. You can push the changes and we can merge the PR now, and change the icon later.

@Schweini07 Schweini07 changed the title Add move tool Add pan tool Dec 22, 2020
@Schweini07
Copy link
Contributor Author

Done!

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@OverloadedOrama OverloadedOrama merged commit 2a7e668 into Orama-Interactive:master Dec 23, 2020
@Schweini07 Schweini07 deleted the move-tool branch December 24, 2020 12:02
OverloadedOrama added a commit that referenced this pull request Jan 17, 2021
The previous icons made by Schweini in #399 will be used for the new move tool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants