-
Notifications
You must be signed in to change notification settings - Fork 489
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
Implement Container.ignore_interactions
property
#3639
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new File-Level Changes
Tips
|
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.
Hey @ndonkoHenri - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
def ignore_interactions(self) -> Optional[bool]: | ||
return self._get_attr("ignoreInteractions", data_type="bool", def_value=False) |
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.
issue (bug_risk): Mismatch in data type for ignore_interactions setter.
The setter for ignore_interactions should accept a value of type Optional[bool] instead of Optional[str] to match the property type.
if (ignoreInteractions) { | ||
result = IgnorePointer(child: result); |
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.
suggestion: Consider using AbsorbPointer instead of IgnorePointer.
AbsorbPointer can be used if you want to block interactions but still allow the widget to be visible to hit testing. This might be more appropriate depending on the use case.
if (ignoreInteractions) { | |
result = IgnorePointer(child: result); | |
if (ignoreInteractions) { | |
result = AbsorbPointer(child: result); |
@@ -197,6 +198,7 @@ def __init__( | |||
self.theme = theme | |||
self.theme_mode = theme_mode | |||
self.color_filter = color_filter | |||
self.ignore_interactions = ignore_interactions |
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.
issue (complexity): Consider simplifying the ignore_interactions
attribute implementation.
The new code introduces unnecessary complexity and redundancy. The ignore_interactions
attribute has inconsistent data types between the getter and setter, which can lead to confusion and potential bugs. Additionally, the added boilerplate code increases the cognitive load. Here's a more concise and consistent approach:
class Container:
def __init__(self, ..., ignore_interactions: Optional[bool] = None, ...):
...
self.ignore_interactions = ignore_interactions
...
@property
def ignore_interactions(self) -> Optional[bool]:
return self._get_attr("ignoreInteractions", data_type="bool", def_value=False)
@ignore_interactions.setter
def ignore_interactions(self, value: Optional[bool]):
self._set_attr("ignoreInteractions", value)
This version ensures consistent data types, reduces boilerplate, and improves code clarity.
Could you resolve conflict here please? |
# Conflicts: # sdk/python/packages/flet-core/src/flet_core/container.py
…nto container-ignore-interactions # Conflicts: # sdk/python/packages/flet-core/src/flet_core/container.py
Summary by Sourcery
This pull request adds a new
ignore_interactions
property to theContainer
class, enabling the option to ignore user interactions. TheContainerControl
class has been updated to handle this property by using anIgnorePointer
widget.ignore_interactions
property to theContainer
class, allowing users to specify whether interactions with the container should be ignored.ContainerControl
class to respect theignore_interactions
property by wrapping the container in anIgnorePointer
widget when the property is set to true.