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

Allow filling rectangles and ellipsis #136

Merged
merged 6 commits into from
Nov 18, 2022
Merged

Conversation

flowln
Copy link
Contributor

@flowln flowln commented Oct 14, 2022

Closes #120

This adds an option on top of existing brush types, to allow for filled shapes in addition to non-filled ones.

I tried to follow the surrounding style as much as possible, but let me know if something isn't quite right 😅

This adds an option on top of existing brush types, to allow for filled
shapes in addition to non-filled ones.

Signed-off-by: flow <flowlnlnln@gmail.com>
This makes it similar to the rectangle draw function.

Signed-off-by: flow <flowlnlnln@gmail.com>
Prevents confusion when using tools that are not affected by this
setting

Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
This is a bit more intuitive i think.

Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
@foxpy
Copy link

foxpy commented Nov 16, 2022

I like it and I would love to see it merged! However, I have two concerns:

  • maybe using toggle button is not the best way to implement this feature? I think checkbox would make more sense? not sure really;
  • I also wish there has been a feature to highlight region instead of filling it completely (maybe just by using opacity in color picker?), though it is out of scope of this PR.

@flowln
Copy link
Contributor Author

flowln commented Nov 16, 2022

I used a toggle button instead of a checkbox because that's what other "toggle-able" UI elements (they'd actually be more akin to radio buttons instead of checkboxes, but the logic still applies i think) in the UI currently use, so I followed for consistency.

For the highlighting, I would think either a field like 'Line Width' and 'Text Size', or a slider thing, could work for selecting an alpha value. As you've said though, that's not really in the scope of these changes, so maybe it could be done at some later point in time 🙂

@jtheoof
Copy link
Owner

jtheoof commented Nov 18, 2022

Thank you @flowln . It's looking good. I will squash all commits into one.

@jtheoof jtheoof merged commit 8ee55f7 into jtheoof:master Nov 18, 2022
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.

[Feature Request] Solid rectangular box
3 participants