Font Library: Change Spinner to ProgressBar component #60570
Conversation
mikachan
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +964 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for the PR.
I tried changing the network throttle to simulate slow internet, but I couldn't reproduce your screenshot. In my environment it was displayed as below:
- Library tab: Theme fonts are displayed below the progress bar.
- Install Fonts tab, a progress bar is displayed below the search bar.
d1f51fa0386aa27ddc2b65b83ff159f3.mp4
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…odal/font-collection.js Co-authored-by: Aki Hamano <[email protected]>
…odal/installed-fonts.js Co-authored-by: Aki Hamano <[email protected]>
Yes, that's the same as what I'm seeing. I've updated the screenshots in the PR description.
This PR was opened to fix #54797, which suggests replacing the Spinner component with the ProgressBar. I'm not too familiar with which component is designed for which area, although I do like this logic:
Perhaps @annezazu or @jasmussen have some insights here 🙏 |
|
Flaky tests detected in 1d41e9e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8650240003
|
I'd tend to echo this instinct. I'll let Anne chime in as well, maybe there's context I'm missing! |
|
I agree! Apologies for the brevity in my initial issue. |
|
Thanks all! In that case, this PR probably isn't needed 😅 However, I think I prefer the spacing that we've worked on here, and centering the Spinner on the "Install Fonts" tab is a possible improvement. I've gone ahead and removed the ProgressBar component, but left the new spacing and alignment. Let me know what you think!
|
mikachan
changed the title
Done, thanks @creativecoder! I also updated a few other details in the PR description. |
|
Thanks for the added context. I think the progressbar is the right one, after all, and that the issue here is that we're using those spinners in a bit of an ad-hoc way. I wonder if this wouldn't be a more ideal flow? 
That is: we don't show anything in any of the tabs, until it's fully loaded. That's one of the benefits of spinners, that you don't have to see the page "getting built" as things load in. Is there any reason the following flow for loading the installed fonts tab couldn't happen? 
By the way, fonts shouldn't wrap:
If you can add |
|
Ah yes, thank you for providing the designs here, I see how the ProgressBar will work. I definitely think this is better. I'm not a fan of the current Spinner setup on the Library tab, especially if the theme provides fonts that visually appear to load in before any other fonts.
I can't think of a technical reason for why this can't happen relatively easily - I'll work on this next in this PR.
I noticed that! They didn't always wrap, I'm not sure what's changed. I'll take a quick look in this PR and if it's not quick I'll spin up something separately. |
mikachan
changed the title
|
I've updated this PR to more closely match the above designs, where:
The text wrapping problem was also easy to fix with the CSS you provided, @jasmussen ✨ This is ready for another review, thank you! |
|
At a quick test, this works well. It's hard to see the spinner when you're testing locally, unless you actively throttle, but that's good. Since this has a green check already, seems good go! Thanks for the work. |
packages/edit-site/src/components/global-styles/font-library-modal/style.scss
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/style.scss
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/installed-fonts.js
Outdated
Show resolved
Hide resolved
# Conflicts: # packages/edit-site/src/components/global-styles/font-library-modal/installed-fonts.js
|
Thanks @t-hamano! This is ready for another review. There are no visual differences to the previous round of changes but I prefer this implementation ✨ |
packages/edit-site/src/components/global-styles/font-library-modal/style.scss
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/installed-fonts.js
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
Outdated
Show resolved
Hide resolved
| display: flex; | ||
| position: absolute; | ||
| left: 0; | ||
| top: $header-height; |
There was a problem hiding this comment.
| top: $header-height; | |
| top: 0; |
It looks like the current loading area doesn't match the size of the modal content. I also think the $header-height CSS variable is inappropriate here since it represents the header size of the editor itself.
I think if we just change it to top:0, the loading area and modal content will match in size and the ProgressBar will be centered exactly.
Before
After
There was a problem hiding this comment.
I used $header-height because this is the variable that's used to set the height of the modal header here.
I think we need to offset the centering position, otherwise the ProgressBar isn't centered within the content area (the area below the header and tabs, and the bottom of the modal):
| Top | Bottom |
|---|---|
Offset by 60px / header-height:
| Top | Bottom |
|---|---|
But I don't think I like using the top position for this, maybe it would be better as a padding-top setting?
I've gone ahead and changed this for now in: 7dafc44.
There was a problem hiding this comment.
I used
$header-heightbecause this is the variable that's used to set the height of the modal header here.
Oh, I forgot that this CSS variable was also used in the modal. However, I believe this is the height of the Guide component's header. I think the height of the Modal component' header is below.
But I don't think I like using the
topposition for this, maybe it would be better as apadding-topsetting?
I think we can adjust either the top or padding-top, but we need to use a value that takes into account both the height of the modal header and the height of the tabs. In other words, it is the sum of the modal height ($header-height + $grid-unit-15) and the tab height ($grid-unit-60). It should be $header-height + $grid-unit-15 + $grid-unit-60 ( 60px + 12px + 48px = 120px).
Below is a screenshot when the value of top remains zero and padding-top: $header-height + $grid-unit-15 + $grid-unit-60 is applied.
There was a problem hiding this comment.
I added the top padding by 89e2314. Please let me know if this works correctly in your environment.
There was a problem hiding this comment.
We don't need to take into account the height of the tabs ($grid-unit-60), as they're set to position: sticky and therefore don't affect the centering calculation. You can see in your above screenshot that this pushes the ProgressBar off-center as it has too much spacing above it.
I did start with the above calculation you stated but noticed that the position of the ProgressBar is incorrect. We only need to account for 60px, which gives us these equal top and bottom measurements:
| Top | Bottom |
|---|---|
There was a problem hiding this comment.
I see, I would like to check it out too, so please push that change 👍
There was a problem hiding this comment.
Hmm, today when I test this, the latest commit using $header-height + $grid-unit-15 + $grid-unit-60 also looks centered. I'm a little confused as this is not what I was seeing before when I first tested using these variables, but I'm happy to go with it if it's working now 😅 I can see in my debug tools that it's coming through as 120px, and it looks correct to me 👍
To confirm, the screenshots in this comment were using the 60px offset value rather than 120px, but also confirming that I'm seeing a nicely centered ProgressBar with 120px ✨
I'm happy to stick with $header-height + $grid-unit-15 + $grid-unit-60 as this makes the most logical sense and also explains itself very nicely, i.e. it's clear why this spacing is needed.
Edit: I've realised why I was seeing different results with $header-height + $grid-unit-15 + $grid-unit-60 last week: it's because I was applying it to top rather than padding-top! 120px doesn't centre correctly when applied to top, but it does when applied to padding-top. I think the padding-top approach is best!
There was a problem hiding this comment.
I'm glad that you seem to have gotten the same results. This PR has already been approved and the code looks good to me, so please feel free to merge 👍
There was a problem hiding this comment.
Thanks so much for your help and reviews here, @t-hamano! I'll go ahead and merge.
What?
This changes any uses of the
Spinnercomponent with theProgressBarcomponent in the Font Library modal.Why?
Fixes #54797.
How?
Swaps one component for another, and also hides the content of the modal until all content has finished loading.
Testing Instructions
Open the Font Library modal and view the Library and Install Fonts tabs, which each use the
ProgressBarcomponent when loading in data.Screenshots or screencast
Library tab:
Install Fonts tab: