Skip to content

USWDS - File Input: Move imageId for larger scope. Resolves #5088 #5114

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.

Sign up for GitHub

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
Merged
thisisdano merged 4 commits into from
May 10, 2023
Merged

USWDS - File Input: Move imageId for larger scope. Resolves #5088 #5114

thisisdano merged 4 commits into from
May 10, 2023

Conversation

@mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Jan 27, 2023 •
edited
Loading

Summary

This change declares imageId outside of the onloadstart() and onloadend() functions to ensure the same target is selected and replaced by the preview image if the attachment takes too long to attach.

Related issue

Closes #5088

Preview link

Preview link: File Input →

Problem statement

If a file takes too long to attach to the file input, it can cause the loading spinner to not be replaced by the preview image.

createUniqueID() uses the current time to create the ImageId variable. When a large file takes time to upload, it can cause onloadstart and onloadend to generate different image IDs.

Solution

By declaring the variable before the functions, we can use createUniqueID() only once and pass the variable through both functions.

Testing and review

  1. Upload larger files to file input // (I had to use a 600mb dmg file to replicate the error)
  2. Confirm that the spinner is replaced with preview image
  3. Make multiple attempts and try file input variants to confirm it still works as expected

@mahoneycm mahoneycm requested review from amyleadem and mejiaj January 27, 2023 21:54
@mahoneycm
Copy link
Contributor Author

mahoneycm commented Feb 3, 2023

@mejiaj @amyleadem Bumping for review if either of you have time today or next week!

@amyleadem
Copy link
Contributor

amyleadem commented Feb 7, 2023

Hi @mahoneycm, I did some preliminary testing and found that the loader now either freezes for a bit before showing the preview image OR doesn’t appear at all. Are you seeing the same thing?
So far I have tested in Chrome, Edge, and Safari and they all have the same issue for me. Here are some screen grabs:

  1. Frozen loader before preview
    75 MB file:

    Screen.Recording.2023-02-07.at.1.32.07.PM.mov

    317 MB file:

    Screen.Recording.2023-02-07.at.1.44.13.PM.mov

  2. No loader before preview:
    75 MB file:

    Screen.Recording.2023-02-07.at.1.35.15.PM.mov

@mejiaj
Copy link
Contributor

mejiaj commented Feb 8, 2023

@amyleadem @mahoneycm I wasn't able to reproduce with a 651MB ZIP (using slow 3G network throttling). Attached video below of current component on develop.

file-input-develop-2023-02-08.at.10.19.45.webm

@mahoneycm
Copy link
Contributor Author

mahoneycm commented Feb 8, 2023

Hi @mahoneycm, I did some preliminary testing and found that the loader now either freezes for a bit before showing the preview image OR doesn’t appear at all. Are you seeing the same thing? So far I have tested in Chrome, Edge, and Safari and they all have the same issue for me. Here are some screen grabs:

  1. Frozen loader before preview
    75 MB file:

    Screen.Recording.2023-02-07.at.1.32.07.PM.mov

    317 MB file:

    Screen.Recording.2023-02-07.at.1.44.13.PM.mov

  2. No loader before preview:
    75 MB file:

    Screen.Recording.2023-02-07.at.1.35.15.PM.mov

@amyleadem Yes I'm seeing the same thing, but only when the icon swap happens successfully. If the attachment takes too long I still see the infinite spinning loader.

Here's a clip where I'm attempting to attach the same file on the current federalist build and on this PR branch.

Screen.Recording.2023-02-08.at.10.43.52.AM.mov

There are times where the network connection is fast enough and the current build does successfully attach the file which results in the spinner pause and file image replacement though. I think if the file upload takes a few second or on the turnover of a new minute it causes the ID's to mismatch and doesn't replace the spinner correctly.

We can take a look at what is causing that pause as well either in this ticket or in a new issue

@mahoneycm
Copy link
Contributor Author

mahoneycm commented Feb 8, 2023

@mejiaj When I first tried to confirm this bug I had trouble recreating it as well. The network throttling didn't seem to affect it for some reason but I kept trying different files and I began seeing the bug. I believe it has to do with the time you start the attachment vs when the attachment ends.

It does seem to be rare though so potentially an edge case.

@mejiaj
Copy link
Contributor

mejiaj commented Feb 8, 2023

@amyleadem with the examples above can you interact at all with the browser? Like right click or select text.

I was able to reproduce the loader icon freezing, but that's because the entire browser was frozen for a second.

@amyleadem
Copy link
Contributor

amyleadem commented Feb 10, 2023

@mejiaj I am not able to interact with the browser when the loader icon is frozen. The browser itself does seem to be frozen during that time.

@mahoneycm
Copy link
Contributor Author

mahoneycm commented Feb 14, 2023

@mejiaj @amyleadem since the freeze seems to be related to the browser freezing, should we go ahead and push this along?

mejiaj
mejiaj approved these changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

The ID is unique and doesn't change once the file is done loading.

@mahoneycm mahoneycm requested a review from thisisdano March 14, 2023 13:44
@mejiaj mejiaj added this to the uswds 3.5.0 milestone Mar 16, 2023
@mahoneycm mahoneycm removed this from the uswds 3.5.0 milestone Mar 29, 2023
@thisisdano
Copy link
Contributor

thisisdano commented May 8, 2023 •
edited
Loading

I'll draw up a changelog PR for this change.
Changelog PR: uswds/uswds-site#2096

@thisisdano thisisdano mentioned this pull request May 8, 2023
Merged
24 tasks
@thisisdano thisisdano merged commit 2a54e86 into develop May 10, 2023
@thisisdano thisisdano deleted the cm-file-input-imageID branch May 10, 2023 21:41
@thisisdano thisisdano mentioned this pull request Jun 6, 2023
Merged
@phildominguez-gsa phildominguez-gsa mentioned this pull request Sep 1, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amyleadem amyleadem Awaiting requested review from amyleadem

2 more reviewers

@mejiaj mejiaj mejiaj approved these changes

@thisisdano thisisdano thisisdano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

USWDS - Bug: Preview Image ID logic issue

5 participants

@mahoneycm @amyleadem @mejiaj @thisisdano