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

Font Library: add progress-bar while uploading font assets #57463

Merged
merged 5 commits into from Jan 8, 2024

Conversation

hbhalodia
Copy link
Contributor

@hbhalodia hbhalodia commented Jan 1, 2024

What?

  • Add ProgressBar component while uploading the font assets.

Why?

How?

  • This PR adds the progress bar components and hide the drop zone and file upload while assets are being uploaded.

Testing Instructions

  1. Open site editor.
  2. Go to styles and then typography option.
  3. Select any style and there would be option for upload.
  4. Add the font file there, you would see the progress bar for the moment till file uploads, and we get any notice.

Testing Instructions for Keyboard

  • None.

Screenshots or screencast

  • Since the uploading is very fast, you would hardly see the progress bar in action.
Font.Library.-.Progerss.Bar.webm

@mikachan mikachan added [Type] Enhancement A suggestion for improvement. [Feature] Typography Font and typography-related issues and PRs labels Jan 3, 2024
Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @hbhalodia!

Currently I'm seeing this in the isUploading state:

image

I think it would be good to centre align the progress bar and add some vertical spacing, similar to the mockup from @jasmussen here.

@hbhalodia
Copy link
Contributor Author

Sure, @mikachan - I would look into this asap.

Thanks.

hbhalodia and others added 2 commits January 4, 2024 09:41
With this commit, we have centered the progress bar and added some vertical spacing to the progress bar so it look as in center.Also while uploading we have just shown the progress bar and no text before and after, because while uploading it is good to show only progress bar.
@hbhalodia
Copy link
Contributor Author

Hi @mikachan, I have revised the code and made the progress bar center aligned and added the vertical spacing. Also in addition to it, I had just showed the progress bar while uploading, since while upload it is good to show just a progress bar and nothing else. Let me know if we need the bottom text while uploading, I can revert that.

Here is the screenshot how it would be while uploading.

Screenshot 2024-01-04 at 10 22 19 AM

Thank You!

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

Thanks @hbhalodia!

Let me know if we need the bottom text while uploading, I can revert that.

After testing this out, I think we should leave this text in place, as it reduces the visual changes and layout shifts.

Maybe we should also move the Notice component below this text, to further reduce the visual changes, e.g.:

image

I've left an inline comment too, as I think we can re-use an existing CSS class rather than the Spacer components. Let me know what you think!

Comment on lines 184 to 190
{ isUploading && (
<FlexItem>
<Spacer margin={ 32 } />
<ProgressBar className="font-library-modal__upload-area__progress-bar" />
<Spacer margin={ 32 } />
</FlexItem>
) }
Copy link
Member

Choose a reason for hiding this comment

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

I think we can achieve a more consistent layout by re-using the font-library-modal__upload-area class, as this already has a fixed height. Something like this:

Suggested change
{ isUploading && (
<FlexItem>
<Spacer margin={ 32 } />
<ProgressBar className="font-library-modal__upload-area__progress-bar" />
<Spacer margin={ 32 } />
</FlexItem>
) }
{ isUploading && (
<FlexItem>
<div className="font-library-modal__upload-area">
<ProgressBar />
</div>
</FlexItem>
) }

We don't want the progress bar to have a grey background, so we could adjust the CSS so the background colour is specific to the upload button:

button.font-library-modal__upload-area {
	background-color: #f0f0f0;

}

This should create a more consistent layout between the not-uploading is isUploading states:

Not-uploading isUploading
image image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure would be looking onto this.

Thanks.

@hbhalodia
Copy link
Contributor Author

I think we should leave this text in place

Sure, Would be keeping as is.

Maybe we should also move the Notice component below this text.

Yep, we can do it. Would update in the next commit.

Also, would address the inline comment that you have mentioned.

Thanks for the review!

Added the consistent style for progress bar and also change the text position before any notice after uploading the fonts in order to reduce the layout shifts.
@hbhalodia
Copy link
Contributor Author

Hi @mikachan, have updated the PR with the required changes, Let me know if that works. Thanks.

Not Uploading State Uploading State
Screenshot 2024-01-04 at 4 59 26 PM Screenshot 2024-01-04 at 5 02 20 PM

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @hbhalodia! ✨

@hbhalodia
Copy link
Contributor Author

Thanks, @mikachan 👍.

@mikachan mikachan merged commit 06e57f2 into WordPress:trunk Jan 8, 2024
55 checks passed
@github-actions github-actions bot added this to the Gutenberg 17.5 milestone Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Font Library: add progress-bar while uploading font assets
2 participants