-
Notifications
You must be signed in to change notification settings - Fork 36
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
Implement skeleton loader (forms, query builder, attachment) #3334
Conversation
…in - attachement gallery Fixes #2998
Triggered by c2ef61c on branch refs/heads/issue-2998
To test:
PS: If 3G not slow enough, you will have to pull the branch and replace the expected data by a temporary hardcoded 'undefined'. *Can also test that animations are disabled for the skeletons when user choose no animation in his preferences |
Preview of a skeleton: form.mov |
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 very nice in the UI
code could use some improvement though
specifyweb/frontend/js_src/lib/components/Forms/SpecifyForm.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AppResource.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AttachmentGallery.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AttachmentGallery.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AttachmentGallery.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/QueryBuilder.tsx
Outdated
Show resolved
Hide resolved
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.
The skeleton is too stark in contrast to the background (black with a while gradient), needs to be something more like this:
Also it is used in place of the loading indicator when the loading circle should still be used after attachments begin to display.
Full video:
https://user-images.githubusercontent.com/37256050/231314507-d62041a1-d632-4f04-8962-fd69fa7eab99.mp4
It would be very nice to have skeleton loading dialogs instead. Forms generally do not have any loading time & some of the components that take a long time to load are:
|
See how this loads into a giant dialog that includes a loading wheel (not needed) then shrinks back to a small window upon completion? When things load quickly, this flashes by and looks bad Screen.Recording.2023-04-11.at.7.13.10.PM.mov |
Triggered by 0e09e7a on branch refs/heads/issue-2998
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AttachmentGallery.tsx
Outdated
Show resolved
Hide resolved
@grantfitzsimmons can you check this version for the previous components and send me your thoughts? |
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.
https://coldfish-issue-2998.test.specifysystems.org/specify/
Open the queries dialog and see that it appears very condensed
There is also an issue with the form skeleton repeating and looking very odd |
Triggered by 4fa777c on branch refs/heads/issue-2998
No way! I refuse to believe it. |
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.
lots of small suggestions, nothing major
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/Skeleton.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/Skeleton.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AppResource.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AppResource.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/Form.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Toolbar/RecordSets.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/DialogList.tsx
Show resolved
Hide resolved
Triggered by 11d2496 on branch refs/heads/issue-2998
All looks good and I will approve this pull request as soon as https://github.com/specify/specify7/pull/3334/files#r1173934603 is fixed. Tell me if you want to meet to talk about that |
yes would be nice |
Added the aria prop to Skeleton.Root |
Triggered by 47fff8c on branch refs/heads/issue-2998
There are still two unresolved comments. Besides those all looks good |
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/AttachmentPlugin.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/SkeletonLoaders/DialogList.tsx
Outdated
Show resolved
Hide resolved
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.
Almost ready!
Just one more thing, the form dialog is really wide by default, can we made this narrower initially?
Screen.Recording.2023-05-04.at.8.27.46.AM.mov
@grantfitzsimmons could you test this PR this week please? |
Fixes #2998