-
Notifications
You must be signed in to change notification settings - Fork 566
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
Foreground function for Container Widget and WidgetExt #2346
Conversation
I left a comment about a test failure. There are also some formatting problems that need to be fixed. There is some valuable information here: https://github.com/linebender/druid/blob/master/CONTRIBUTING.md Lastly, don't forget to add your PR to the changelog. You can also add yourself to the authors file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks fine, but I've got some inline comments.
Also, when you're done with the changes don't forget to run cargo fmt
.
@@ -77,6 +79,36 @@ impl<T: Data> Container<T> { | |||
self.background = None; | |||
} | |||
|
|||
/// Builder-style method for setting the foreground for this widget. | |||
/// | |||
/// This can be passed anything which can be represented by a [`BackgroundBrush`]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess BackgroundBrush
is no longer a great name. Not a change we need to worry about in this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking the same since I was preparing the PR. Since this didn't make it in time should I prepare another PR for the BackgroundBrush
change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wish, sure. After we merge this one here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I just rename it to Brush
? I am also thinking PrimitivesBrush
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brush
sort-of clashes with Piet's Brush
so perhaps not. Given that it is defined in painter.rs
I'm also thinking maybe PaintBrush
. Although we could maybe go with something non-brush even, given that it supports a full Painter
widget as a variant. 🤔
Co-authored-by: Kaur Kuut <strom@nevermore.ee>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a CHANGELOG.md
entry for these changes. You probably need to rebase on master
first to avoid merge conflicts.
The main entry in the unreleased version's added section at the top of the file:
### Added
- `foreground`, `set_foreground`, and `clear_foreground` methods to `Container` and `WidgetExt::foreground` method for convenience. ([#2346] by [@giannissc])
Then also define your profile link and the PR link at the very bottom of the file.
[@giannissc]: https://github.com/giannissc
[...]
[#2346]: https://github.com/linebender/druid/pull/2346
Ready to be merged! @xStrom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
No description provided.