Skip to content
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

Replace react content loader? #423

Closed
Hyperkid123 opened this issue Mar 26, 2020 · 4 comments
Closed

Replace react content loader? #423

Hyperkid123 opened this issue Mar 26, 2020 · 4 comments
Labels
question Further information is requested Stale utils update to utils package

Comments

@Hyperkid123
Copy link
Contributor

Hyperkid123 commented Mar 26, 2020

I have recently encountered a bug with content loader in safari browser (info here: danilowoz/react-content-loader#93)

The issue is that the content loader is using URL for the SVG fill attribute. The proposed fix with the baseUrl prop did fix it in Safari but broke it in Chrome on Linux. I was using a newer version than is used here in the utils package. Combine that with very limited responsiveness of SVG on different screen sizes, I decided to remove this dependency and implement the loader myself using just CSS. You can see the implementation in this PR: https://github.com/RedHatInsights/catalog-ui/pull/484/files#diff-07c73903e209a4a97b1f0a9c93a44f2fR26-R77

This approach is working very well, the issues with browser compatibility are gone and I can achieve much greater detail and customization than with SVG variant while keeping the loader responsive.

I would like to propose abandoning this dependency and CSS implementation to replace skeletons in the utils package. It does not have to be exactly the same as the one implemented in the catalog and there are probably some shapes missing, but that is something that can be very easily added.

@ryelo @karelhala @RedHatInsights/ui-reviewers Your thoughts?

@Hyperkid123 Hyperkid123 added enhancement New feature or request question Further information is requested utils update to utils package and removed enhancement New feature or request labels Mar 26, 2020
@rvsia
Copy link
Contributor

rvsia commented Mar 26, 2020

Maybe we can leave using of content loader at all and use only spinners as Patternfly suggests. https://www.patternfly.org/v4/design-guidelines/usage-and-behavior/data-loading It's much easier to use a spinner instead of trying to create a proper SVG placeholder.

@ryelo
Copy link
Member

ryelo commented Mar 26, 2020

We're working on a standard for loaders versus spinners versus skeletons.

To sum it up, we should ditch these common loaders entirely because they don't represent the data loading appropriately. Ideally, we would create a skeleton version of components, filling in constant data (like table headers), and then showing a spinner/skeleton when the data is loading -- depending on the shape and how much we know about it (skeleton when you know the shape, spinner when we don't).

I can share more info later when we finish creating the specs.

@Hyperkid123
Copy link
Contributor Author

Hyperkid123 commented Mar 26, 2020

I agree with what @ryelo posted. If it's decided that there will be some animated skeletons, I would personally prefer to go with CSS implementation over some animated SVGs.

Feel free to close this issue. I assume the standards will include browser compatibility.

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested Stale utils update to utils package
Projects
None yet
Development

No branches or pull requests

3 participants