-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add much more detailed comments throughout the new atom-based radio_button widget #7496
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
Conversation
Added button and checkbox documentation (previously added radio_button documentation).
use crate::{ | ||
Atom, AtomExt as _, AtomKind, AtomLayout, AtomLayoutResponse, Color32, CornerRadius, Frame, | ||
Image, IntoAtoms, NumExt as _, Response, Sense, Stroke, TextStyle, TextWrapMode, Ui, Vec2, | ||
Widget, WidgetInfo, WidgetText, WidgetType, |
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.
are these comments really needed??
this so much more lines which I don't really think we need
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 think having a heavily documented widget is helpful for those that are new to rust, or who are finding their way to how widgets work, or who require a more in-depth explanation.
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 agree that comments inside functions and doc-comments over it are helpful (I didn't read those ones), but commenting of imports is superfluous. That documentation should be on the imported things itself.
@@ -1,137 +1,311 @@ | |||
// Import necessary types and traits from the crate | |||
use crate::{ | |||
Atom, AtomLayout, Atoms, Id, IntoAtoms, NumExt as _, Response, Sense, Shape, Ui, Vec2, Widget, |
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.
same here
// Import necessary types and traits from the crate | ||
use crate::{ | ||
Atom, AtomLayout, Atoms, Id, IntoAtoms, NumExt as _, Response, Sense, Ui, Vec2, Widget, | ||
WidgetInfo, WidgetType, epaint, |
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.
same 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.
I feel it adds a lot of noise with little value, as ai tends to do. I don't think we should merge it, but we can keep the PR around, (maybe even link it from somewhere), in case someone wants to read this.
// Get visual styling based on the widget's interaction state | ||
// Note: We use the general interact() method instead of interact_selectable() | ||
// because the selectable version can be "too colorful" according to the comment |
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.
Lol, according to what comment?
// Small buttons get no vertical padding | ||
if small { | ||
button_padding.y = 0.0; | ||
} |
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 feel like the majority of code comments don't add much value. Do you really need a comment to know that this disables vertical padding for small buttons?
In general, code should be written in a way that makes it obvious what is going on, and comments should explain things that aren't obvious from the code, for example why something is done.
If you think these comments are useful, then you can generate them locally (like you did). No need to commit them. |
this just feels like copilot was told to comment the project xD |
The goal of this pull request is to add many more detailed comments to the
radio_button
widget code to help better understand the new atom-based widgets.A visual diff has been completed and the demo app has been compiled to check for errors or warning:
cargo run --release -p egui_demo_app
The comments were written by Claude AI and have been entirely checked to the best of my ability.