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.

Sign up for GitHub

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

Jump to bottom

[reopen] Rename NavigationDestination to NavigationBarDestination + some type hinting, fixes and new features #3172

Merged
merged 29 commits into from
May 30, 2024
Merged

[reopen] Rename NavigationDestination to NavigationBarDestination + some type hinting, fixes and new features #3172

merged 29 commits into from
May 30, 2024

Conversation

bleudev
Copy link
Contributor

@bleudev bleudev commented May 2, 2024 •
edited by ndonkoHenri
Loading

Checklist:

  • Navigation bar's NavigationDestination renamed to NavigationBarDestination.

  • Type hinting:

    • Add type alias:
      • Number
      • ControlEventFunction
      • IconStr
      • ColorStr
      • Wrapper
    • navigation_bar.py, navigation_drawer.py, navigation_rail.py
    • cupertino_navigation_bar.py
    • page.py
    • control.py

  • Move OptionalNumber to types.py

  • page.py:

  • Magic methods:

    • Control in Page

  • isort and black formatting

(closes #3173)
(reopen of #3161)

It should be merged with #281

@bleudev bleudev changed the title [reopen] Rename NavigationDesttination to NavigationBarDestination + some type hinting and fixes [reopen] Rename NavigationDestination to NavigationBarDestination + some type hinting and fixes May 2, 2024
@bleudev
Copy link
Contributor Author

bleudev commented May 2, 2024 •
edited
Loading

@ndonkoHenri Do i can merge your pull (#3053) into my pull (write your code to my pull)? then it will be more convenient. Author of project will have to merge only my pull

@bleudev bleudev changed the title [reopen] Rename NavigationDestination to NavigationBarDestination + some type hinting and fixes [reopen] Rename NavigationDestination to NavigationBarDestination + some type hinting, fixes and new features May 3, 2024
bleudev and others added 10 commits May 4, 2024 00:47
@bleudev bleudev marked this pull request as ready for review May 4, 2024 23:01
@bleudev bleudev requested a review from ndonkoHenri May 5, 2024 08:14
@bleudev
Copy link
Contributor Author

bleudev commented May 5, 2024

@FeodorFitsner Can you review this PR?

@ndonkoHenri
Copy link
Contributor

@ndonkoHenri Do i can merge your pull (#3053) into my pull (write your code to my pull)? then it will be more convenient. Author of project will have to merge only my pull

Let's keep everything separate. It eases review.

@bleudev
Copy link
Contributor Author

bleudev commented May 6, 2024

@ndonkoHenri Do i can merge your pull (#3053) into my pull (write your code to my pull)? then it will be more convenient. Author of project will have to merge only my pull

Let's keep everything separate. It eases review.

I have already implemented this :)

@ndonkoHenri
Copy link
Contributor

I have already implemented this

Alright, didn't see that before.
Please next time let's keep things separate when possible :)

Furthermore, Autoupdate is actually still in discussion, reason why I didn't wrap it up.

@bleudev
Copy link
Contributor Author

bleudev commented May 6, 2024 •
edited
Loading

I have already implemented this

Alright, didn't see that before. Please next time let's keep things separate when possible :)

Furthermore, Autoupdate is actually still in discussion, reason why I didn't wrap it up.

When my pull will be merged, your features will also be merged. Can you review my pull for to make it happen faster?

@bleudev bleudev mentioned this pull request May 8, 2024
Closed
@FeodorFitsner
Copy link
Contributor

@bleudev, @ndonkoHenri So this PR is based on a branch from #3053?

@FeodorFitsner
Copy link
Contributor

FYI: Created a new channel for devs: https://discord.com/channels/981374556059086931/1243237230080688199

@FeodorFitsner FeodorFitsner mentioned this pull request May 23, 2024
Merged
@FeodorFitsner
Copy link
Contributor

Great PR with valuable typings additions - much appreciated!

@bleudev
Copy link
Contributor Author

bleudev commented May 24, 2024

Great PR with valuable typings additions - much appreciated!

Thanks

@ndonkoHenri
Copy link
Contributor

#3348
I personally think we should leave colors and icons as-is. I mean, what is honestly the issue with Optional[str] ? it's short and normal - nothing fancy, no?
However, if you dont think its the case, then I suggest you could create something like OptionalString=Optional[str], then use it where necessary. I dont see the need of having HexStr, ColorStr, IconStr, etc, which are at the end of the day thesame (= Optional[str])

@bleudev
Copy link
Contributor Author

bleudev commented May 27, 2024

#3348

I personally think we should leave colors and icons as-is. I mean, what is honestly the issue with Optional[str] ? it's short and normal - nothing fancy, no?

However, if you dont think its the case, then I suggest you could create something like OptionalString=Optional[str], then use it where necessary. I dont see the need of having HexStr, ColorStr, IconStr, etc, which are at the end of the day thesame (= Optional[str])

I became wanting this because feodor said about it in review above. I think that it's very good idea because it's simplifying type hinting where is using icon strings. Idea with ColorStr is not worked out. I think that we should add IconStr enum, but we shouldn't do something with ColorStr right now.

@bleudev bleudev mentioned this pull request May 27, 2024
Closed
@bleudev
Copy link
Contributor Author

bleudev commented May 29, 2024

@FeodorFitsner re-review please)

@FeodorFitsner
Copy link
Contributor

Reviewing it now. Thanks!

@FeodorFitsner FeodorFitsner merged commit f008f39 into flet-dev:main May 30, 2024
2 checks passed
@bleudev
Copy link
Contributor Author

bleudev commented May 31, 2024

Thank you very much!

@bleudev bleudev mentioned this pull request May 31, 2024
Closed
zrr1999 pushed a commit to zrr1999/flet that referenced this pull request Jul 17, 2024
@bleudev @zrr1999
[reopen] Rename NavigationDestination to NavigationBarDestination…
eb7d031 
… + some type hinting, fixes and new features (flet-dev#3172)

* init commit

* `page.py` type hinting part 1, `close_snack_bar()`, Snack bar and Dialog open/close methods fixes

* Fix Inherited

* Fix `'type' object is not subscriptable`

* type hinting `page.py` final

* `auto_update` + `Page.__contains__()`

* add `@deprecated` for`NavigationDestination`, `add()`, `insert()`, `remove()` methods for `NavigationBar` and magic methods (`Control.__iadd__()`, `Control.__add__()`, `Control.__radd__()`, `Control.__isub__()`, `Control.__sub__()`, `Control.__rsub__()`, `Control.__setitem__()`

* `add()`, `remove()`, `inserrt()`: `NavigationDrawer` and `NavigationRail`

* `CupertinoNavigationBar`, `View`, `Column`, `Row`

* Type hinting `control.py`

* Fix `NameError: name 'Page' is not defined
`

* Final `add()`, `insert()`, `remove()`

* type hinting for `cupertino_navigation_bar.py`

* isort and black formatting

finnaly i made this pull

* Change `0.22.0` to `0.22.1` in deprecations

* Fix `except ImportError`

* Remove `add/insert/remove` methods + remove magic methods

* Fix `SyntaxError` and fix `Optional`

* Fix `TypeError`

* Remove `Args` and `Kwargs` from `types.py`

* Delete auto_update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FeodorFitsner FeodorFitsner FeodorFitsner approved these changes

@ndonkoHenri ndonkoHenri Awaiting requested review from ndonkoHenri

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add possibility to use Page.show_...() methods without argument
3 participants
@bleudev @ndonkoHenri @FeodorFitsner