-
Notifications
You must be signed in to change notification settings - Fork 979
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 loading screen for fetching community #19968
Fix loading screen for fetching community #19968
Conversation
Jenkins BuildsClick to see older builds (24)
|
76b7b49
to
b4d8983
Compare
I am not sure if we need design review for this PR,as these designs are not implemented by design team 🤔 |
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.
Thank you!
a small preference, but i'd put the text below the image instead of the header, as I think the attention will be in the centre because of the cat 🐱. WDYT?
Thank you @clauxx for reviewing the PR.
Yes, this is what I had initially. But without title, close button looks weird. Maybe we can use some other title for the screen 🤔. Any suggestions? |
Community screen? or Community Overview? |
IMO it looks fine without a title, but up to you. |
85% of end-end tests have passed
Failed tests (6)Click to expandClass TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
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.
Looks good ✅
I left one tiny comment 👍
And maybe we should double-check with design about the header title?
25e284d
to
6940f90
Compare
Thank you @seanstrom for reviewing the PR.
Hi @Francesca-G as designs for loading screen while fetching community is not available, I have created this PR to show a temporary screen and get rid of red box. (Please check out PR description for screenshots). Please let me know, if it looks ok or needs to be updated. Thank you |
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.
5e47966
to
b47482a
Compare
Thank you @Francesca-G for checking the PR.
Updated failed state, thank you |
@Parveshdhull thank you for the PR. No issues from my side. After @Francesca-G approves PR is ready for merge. |
Thank you very much @pavloburykh for testing the PR. I think @Francesca-G already approved the PR, so its ok to merge. |
b47482a
to
2fc4b9d
Compare
2fc4b9d
to
e0bb1d6
Compare
[quo/empty-state | ||
{:image (resources/get-themed-image :cat-in-box theme) | ||
:description (when-not fetching? (i18n/label :t/here-is-a-cat-in-a-box-instead)) | ||
:title (if fetching? "Fetching community..." "Failed to fetch community") |
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.
@Parveshdhull, shouldn't this be in our translation files?
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.
Ah yeah good catch 🙏
Most likely should be in the translation file
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.
hi @ajayesivan, @seanstrom, Thank you for reviewing PR.
I didn't move them intentionally, as they were already in the file and I assumed that's what we do when we don't have proper designs and implementation is just temporary.
wdyt, should we move them to the translations file or handle them when implementing this placeholder when we have designs?
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 see what you mean about these labels being temporary 👍
My only thought is that if this UI can be seen without a feature flag, then maybe the labels should be translatable. And later on if we want to change them we can decide to remove them or not. How does that sound?
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.
sounds good, will move them to translations file in one of my PRs. thank you
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.
Thank you @Parveshdhull 🙏
fixes #19555
Summary
Please let me know if this looks ok or needs to be improved.
Testing
Please lightly test PR for fetching community screen and let me know if anything needs to be improved. (including design)
status: ready