-
-
Notifications
You must be signed in to change notification settings - Fork 252
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
feat: Add view controllers #407
Conversation
✅ Deploy Preview for robyn canceled.
|
Thanks for the PRs @mikaeelghr . It was amazing waking up to these happy surprises 😄 I will try to review your PRs over the course of the day. |
Hey @mikaeelghr , Thanks for the PR. I have a few suggestions for the PR. Can you write some integration tests for your code? Second instead of creating an But if they want to organize their code in different files or responsibilities, they should use the view syntax and they can include the function from the module. |
@mikaeelghr , I have added a few tests. Everything else looks great! |
22475fa
to
a17611f
Compare
Thanks for the follow up @sansyrox, I was very busy this week and I couldn't add the tests. One more thing, based on your comment, shouldn't we delete the |
No worries @mikaeelghr 😄 I am actually working a lot with views today. And I realized that having this versatility(the I will be pushing an update shortly |
abb6e0e
to
7038cb2
Compare
ceff604
to
9f869b6
Compare
d8e574e
to
b5ad609
Compare
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.
Great job! 😄 I added a few small suggestions. One last here: I would put all tests related to views in the same file, be they sync or async. This is what's done for the other features so I think it makes more sense.
for more information, see https://pre-commit.ci
fba317e
to
88bf42a
Compare
@AntoineRR , everything is resolved now. Thank you for the review 😄 Thanks for the great work again @mikaeelghr 😄 |
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 for the fixes! One last thing @sansyrox, can't we merge the integration_tests/views/async_view.py
and integration_tests/views/test_view.py
files together? Or at least rename test_view.py
to sync_view.py
?
We can but I wanted to suggest one view per file(like react) for better organization of the code.
Done! 😄 |
@mikaeelghr @sansyrox Genuine question, why not use a simple |
Hi @domingues 👋 We don't want to choose function base approach over class based approach. I just like to structure my code in closures sometimes and not having the ability to do that anywhere else was annoying . We just haven't got around to implementing class-based views as of yet. 😅 |
Description
Add the ability to add views.
usage:
This PR fixes #221