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

Jump to bottom

Marketplace: modernize skeleton loaders #40131

Merged
merged 1 commit into from Oct 16, 2023
Merged

Marketplace: modernize skeleton loaders #40131

merged 1 commit into from Oct 16, 2023

Conversation

Dan-Q
Copy link
Contributor

@Dan-Q Dan-Q commented Sep 12, 2023 •
edited

Changes proposed in this Pull Request:

Closes WCCOM#18034. Closes WCCOM#18257.

This PR replaces all "skeleton" loading indicators on the Marketplace with more-standard ones. It also aims to make those skeleton loaders:

  • More-accurately represent this size and shape of the content that will replace them, and
  • Refactors the code so that components are responsible for hosting their own skeleton code, attached to an isLoading variable, making it easier to stay consistent as changes are made in future

How to test the changes in this Pull Request:

  1. Check out this branch and ensure everything's recompiled with e.g. cd plugins/woocommerce-admin && pnpm run start:hot
  2. Visit http://localhost:8888/wp-admin/admin.php?page=wc-admin&path=%2Fextensions and observe the new skeleton loaders on the Discover page
    3.Visit http://localhost:8888/wp-admin/admin.php?page=wc-admin&path=%2Fextensions&tab=extensions and see the new skeleton loaders on the Browse page (you might also like to do a search)

Demo video

This demo video artificially slows down loading a lot so the skeletons are very visible:

Screen.Capture.on.2023-10-12.at.14-40-11.mp4

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

Improved loading indicators on WooCommerce > Extensions so they more-accurately reflect the layout of the content that will replace them.

@Dan-Q Dan-Q added the needs: developer feedback Issues that need feedback from one of the WooCommerce Core developers. label Sep 12, 2023
@Dan-Q Dan-Q requested a review from a team September 12, 2023 13:25
@Dan-Q Dan-Q self-assigned this Sep 12, 2023
@Dan-Q Dan-Q requested review from bgrgicak and kdevnel and removed request for a team September 12, 2023 13:25
@github-actions
Copy link
Contributor

github-actions bot commented Sep 12, 2023 •
edited

Hi @bgrgicak, @kdevnel, @poligilad-auto,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Contributor

github-actions bot commented Sep 12, 2023 •
edited

Test Results Summary

Commit SHA: 03ba981

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 54s
E2E Tests212004021625m 25s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

kdevnel

This comment was marked as resolved.

Sign in to view

bgrgicak

This comment was marked as resolved.

Sign in to view

@kdevnel

This comment was marked as resolved.

Sign in to view
@bgrgicak

This comment was marked as resolved.

Sign in to view
@poligilad-auto

This comment was marked as resolved.

Sign in to view
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Oct 12, 2023
@Dan-Q Dan-Q marked this pull request as draft October 12, 2023 13:21
@Dan-Q
Copy link
Contributor Author

Dan-Q commented Oct 12, 2023

I've addressed all of the feedback, so I'd love any more you can provide:

  • @kdevnel I've improved the layout of the code and satisfied the linter.
  • @bgrgicak I've used SASS variables where applicable. Re: the design, I'm addressing feedback by @poligilad-auto.
  • @poligilad-auto As requested this now uses a much simpler layout with large blocks rather than individual lines of text. The individual elements (icon/image, title, price, description) still each have their own blocks, which makes it possible for the user to determine what kind of item will load there. I've added a video to the PR description (with dramatically slowed-down loading to make them very visible!) for your thoughts.
  • I've also made it all compatible with my load cacher (which was merged to trunk earlier today) and with theme cards (which were merged to trunk after this PR was first created)

This now also fixes WCCOM#18257 as a skeleton appears in place of the "0 extensions" etc. titles while loading is occuring.

Hopefully that covers everything!

@Dan-Q Dan-Q marked this pull request as ready for review October 12, 2023 13:49
bgrgicak
bgrgicak approved these changes Oct 12, 2023
View reviewed changes
plugins/woocommerce-admin/client/marketplace/components/product-card/product-card.scss
.woocommerce-marketplace__product-card__vendor,
.woocommerce-marketplace__product-card__description,
.woocommerce-marketplace__product-card__price {
height: calc(13px * 1.5);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to store values like these in variables so that there is at least some description to what they mean.
Otherwise, we can just have the final value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this, but calc didn't like calculating with a var in the other place we use this value, so I just added a couple of comments.

andfinally
andfinally approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@andfinally andfinally left a comment

Choose a reason for hiding this comment

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

Looks lovely, thanks! Pushed a couple of small tweaks in 3ab2db07cec4cdb46d1cec112318044ebaa1e875 and f5725288cd175dfdf1e8880e5a3d6358ba57acc9 – when you call Array.map the index is the second param.

If the tests pass I might have a go at squashing this PR and merging.

@andfinally
This branch replaces all "skeleton" loading indicators on the Marketp…
03ba981 
…lace with more-standard ones. It also:

- Aims to make those skeleton loaders more accurately represent this size and shape of the content that will replace them.
- Refactors the code so that components are responsible for hosting their own skeleton code, attached to an `isLoading` variable, making it easier to stay consistent as changes are made in future.
@andfinally andfinally force-pushed the fix/wccom-18034-modern-skeleton-loaders branch from f572528 to 03ba981 Compare October 16, 2023 15:10
@andfinally andfinally removed the needs: developer feedback Issues that need feedback from one of the WooCommerce Core developers. label Oct 16, 2023
@andfinally andfinally dismissed kdevnel’s stale review October 16, 2023 16:18

Dan has addressed the points in Kyle's review.

@andfinally andfinally merged commit 0389761 into trunk Oct 16, 2023
26 checks passed
@andfinally andfinally deleted the fix/wccom-18034-modern-skeleton-loaders branch October 16, 2023 16:18
@github-actions github-actions bot added this to the 8.3.0 milestone Oct 16, 2023
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Oct 16, 2023
@alopezari alopezari added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andfinally andfinally andfinally approved these changes

@bgrgicak bgrgicak bgrgicak approved these changes

@kdevnel kdevnel Awaiting requested review from kdevnel

@poligilad-auto poligilad-auto Awaiting requested review from poligilad-auto

Assignees

@Dan-Q Dan-Q

Labels
needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Milestone
8.3.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Dan-Q @kdevnel @bgrgicak @poligilad-auto @andfinally @alopezari