-
Notifications
You must be signed in to change notification settings - Fork 332
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
fix: user detail broken in demo (#59) #60
Conversation
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.
otherwise LGTM 👍.
python/demo/tables.py
Outdated
|
||
@router.get('/users/{id}/', response_model=FastUI, response_model_exclude_none=True) | ||
def user_profile(id: int) -> list[AnyComponent]: | ||
users = [ |
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.
please define these globally, then reuse them.
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 have resolved this and changed the class name MyTableRow
to User
for better understanding.
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.
users: list[User]
is now a global variable to access from everywhere.
python/demo/tables.py
Outdated
return demo_page( | ||
*tabs(), | ||
c.Link(components=[c.Text(text='Back')], on_click=BackEvent()), | ||
c.Details(data=user), |
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.
You need to add fields
here so the data is rendered correctly.
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.
Done adding fields
here and also added in city details for better explicit controls.
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 there's no need on city as all the columns are simply text.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 120 120
=========================================
Hits 120 120 |
this is great, thanks so much. |
As the issue #59 mentioned about the broken user detail page. I fixed the page and fixed the url from
/more/{id}
to/table/users/{id}
. Now, It looks more relevant than/more
route.Hope it make sense.