-
Notifications
You must be signed in to change notification settings - Fork 449
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
CHORE: Move page.window_*
and page.browser_context_menu_*
properties to Window
and BrowserContextMenu
classes
#3463
Conversation
Reviewer's Guide by SourceryThis pull request refactors the 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 found some issues that need to be addressed.
Blocking issues:
- Typo in property name (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 2 other issues
- 🟢 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.
self.page.update() | ||
|
||
def center(self) -> None: | ||
self.page._set_attr("windowCenter", str(time.time())) |
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: Use of time.time()
for unique values
Using time.time()
to generate unique values might not be reliable in all cases. Consider using a more robust method for generating unique identifiers.
self.page._set_attr("windowCenter", str(time.time())) | |
import uuid | |
self.page._set_attr("windowCenter", str(uuid.uuid4())) |
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.
@FeodorFitsner, wdyt?
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.
That makes sense.
sdk/python/packages/flet-core/src/flet_core/utils/deprecated.py
Outdated
Show resolved
Hide resolved
* add type hint * fix Future[RetT]
…ies to `Window` and `BrowserContextMenu` classes (flet-dev#3463) * initial commit * commit * update Window * create BrowserContextMenu class * chore: remove deprecation_warning and reformat deprecated * change signal to SIG_DFL after exit_gracefully (flet-dev#3466) * improve type hint for run_task and run_thread (flet-dev#3459) * add type hint * fix Future[RetT] * feat(map): add missing py-events, better typing (flet-dev#3464) * initial commit * commit * update Window * create BrowserContextMenu class * chore: remove deprecation_warning and reformat deprecated * Geolocator: rename some classes and methods --------- Co-authored-by: Zhan Rongrui <46243324+zrr1999@users.noreply.github.com>
Description
page.window_*
andpage.browser_context_menu_*
properties toWindow
andBrowserContextMenu
classes respectivelypage.window_*
props in favor ofpage.window.*
shadow
,alignment
,wait_until_ready_to_show
,always_on_bottom
,icon
,badge_label
)Fixes #3414
Test Code
Type of change
Checklist:
Screenshots (if applicable):
Additional details
page.window.icon
was added but not tested since it is windows-only (which I don't have)