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

Render Skeleton When Loading Profile Viewer #152

Merged
merged 6 commits into from
Aug 19, 2021
Merged

Render Skeleton When Loading Profile Viewer #152

merged 6 commits into from
Aug 19, 2021

Conversation

hreineck
Copy link
Contributor

Modified the way the profile viewer loads to show only the skeleton
card when the page is loading in; this fixes the issues with the
skeleton not rendering at all, as well as making it a bit more clear
that the page is still loading content.

Modified the way the profile viewer loads to show only the skeleton
card when the page is loading in; this fixes the issues with the
skeleton not rendering at all, as well as making it a bit more clear
that the page is still loading content.
The Back Matter and Metadata components previously were rendered
alonside the skeleton. Now that those components do not render right
away, the tests for those components had to be updated; the
test functions were changed to use `await` and a timeout of 10000ms
to wait on the page to load before finding the elements.
Copy link
Contributor

@rgauss rgauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the skeletons should be used in each area of the page that might take time to independently load and should resemble the general shape of the component that will be rendered, populating independently as the data is available.

@hreineck hreineck changed the title Show Only Skeleton When Loading Profile Viewer Render Skeleton When Loading Profile Viewer Aug 12, 2021
When the controls are still being resolved, display the
back matter and metadata components alongside the skeleton,
because they will be fully loaded before the controls are resolved.
@hreineck
Copy link
Contributor Author

hreineck commented Aug 12, 2021

In the previous commit that initially sets up the skeleton,
there is an array of 3 skeleton cards rendered. I felt the single card looked a bit better, but if rendering multiple cards is desired that can be implemented instead.

@hreineck hreineck requested a review from rgauss August 12, 2021 15:02
@hreineck hreineck marked this pull request as ready for review August 12, 2021 15:02
Change the skeleton to render two rectangles in the place of
the "imported controls." This more accurately matches the style
of the page when it loads.
@hreineck hreineck requested a review from rgauss August 12, 2021 18:26
Copy link
Contributor

@rgauss rgauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, looked better before my terrible suggestion 😟.

@hreineck hreineck requested a review from rgauss August 12, 2021 20:30
Copy link
Contributor

@rgauss rgauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, provided that the distribution (commands the pipeline execute) displays things properly.

Copy link
Contributor

@kkennedy26 kkennedy26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing the two different styles, it definitely looks better this way having the skeleton for loading imported controls instead of the whole page. Also, it isn't part of this task, but there is a console error for a key prop when running tests. I don't know if there is an issue for it already.

@tuckerzp
Copy link
Contributor

Also, it isn't part of this task, but there is a console error for a key prop when running tests.

Out of curiosity, was console error there before this branch was created? Just don't want to unintentionally add errors to fix later.

@kkennedy26
Copy link
Contributor

Out of curiosity, was console error there before this branch was created? Just don't want to unintentionally add errors to fix later.

It must have been there before this branch, but not exactly sure where or when this error occurred.

Copy link
Contributor

@mikeisen1 mikeisen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks so much better with the skeleton rendering than without. Great work!

@mikeisen1 mikeisen1 merged commit 589f06e into develop Aug 19, 2021
@rgauss rgauss deleted the EGRC-450 branch August 25, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants