-
Notifications
You must be signed in to change notification settings - Fork 7
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 unable to view loading circular progress #4043
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.
Got a question for you
@@ -34,6 +34,10 @@ const MuiButton = styled(MuiIconButton)` | |||
color: #666666; | |||
`; | |||
|
|||
const EmptyView = styled.div` | |||
height: 50vh; |
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.
Where does 50vh come from? Seems pretty magic to me, and I can't see what it relates to...
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.
Yeah it is a very special case for LoadingContainer
. EmptyView
will help us to control the height of LoadingContainer
dynamically.
The first thing is the height of LoadingContainer
is the height of its child component. The existing problem is LoadingContainer
current child isExportView
, and its height is fixed, so when loading, if client has a small screen they cannot see the loading circular progress, unless they drag down.
I choose to adapt the behaviour of LoadingContainer
since it got used by many places.
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.
Right I get it!
How about something like this? https://github.com/beyondessential/tupaia/pull/4048/files
The EmptyView
wasn't as clear to me as an "isLoading" prop that defines the different styles
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's much clearer @edmofro thanks! Nice one.
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.
Nice changes ;-)
Issue #:
https://linear.app/bes/issue/RN-476#comment-5192a9d2