Adds placeholder-style loading to the ServiceItems #932
Conversation
use new style in ServiceItem loaders
| import LoadingPlaceholder from "./LoadingPlaceholder"; | ||
|
|
||
| export enum LoaderType { | ||
| Spinner, |
|
|
||
| export interface ILoadingWrapperProps { | ||
| type?: LoaderType; |
There was a problem hiding this comment.
I am a little bit worried about using such an overloaded term type. It does not seem a reserved word in JS though.
| import LoadingSpinner from "../LoadingSpinner"; | ||
| import LoadingPlaceholder from "./LoadingPlaceholder"; |
There was a problem hiding this comment.
Maybe this should be a little bit more specific?
LoadingTextPlaceholder?
There was a problem hiding this comment.
I feel like the "placeholder" is a style in itself, just search for "placeholder loader" on Google and you come up with exactly what this is: https://www.google.com/search?q=placeholder+loader&oq=placeholder+loader&aqs=chrome.0.69i59j69i60j0l4.3666j1j7&sourceid=chrome&ie=UTF-8.
| @@ -0,0 +1,25 @@ | |||
| .LoadingPlaceholder { | |||
| height: 1em; | |||
| background-color: #f1f1f1; | |||
There was a problem hiding this comment.
Are you sure that this is the minimum amount of CSS that we need? Do we need the background color set here?
There was a problem hiding this comment.
yes, this is the grey background colour that the lighter one animates over
|
|
||
| // Renders a placeholder loader style | ||
| // Based on https://kyusuf.com/post/fake-it-til-you-make-it-css/ | ||
| const LoadingPlaceholder: React.SFC = props => { |
There was a problem hiding this comment.
You are missing tests, even if it is a simple snapshot one.
There was a problem hiding this comment.
I saw that the LoadingSpinner didn't have tests, but I can add them for this.
There was a problem hiding this comment.
Not a lot to test but I guess it's something nice to have
|
|
||
| // Renders a placeholder loader style | ||
| // Based on https://kyusuf.com/post/fake-it-til-you-make-it-css/ | ||
| const LoadingPlaceholder: React.SFC = props => { |
There was a problem hiding this comment.
Not a lot to test but I guess it's something nice to have
The placeholder style loading is used in some webapps to show that text
content is loading. This is a better fit for the ServiceTables compared
to the LoadingSpinner.
The implementation adds a new
typeprop to the LoadingWrappercomponent, which is used to determine which type of loader to render.
This could easily be extended to different types of loaders in the
future.