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

Proper state/error management for file ingestion #639

Merged
merged 21 commits into from
Nov 18, 2024
Merged

Proper state/error management for file ingestion #639

merged 21 commits into from
Nov 18, 2024

Conversation

roll
Copy link
Collaborator

@roll roll commented Nov 12, 2024


@romicolman
Please take a look, it fully reworks ingestion data flow and logic that properly covers:

  • loading states
  • error handling
  • blocking states (you can't click other buttons while it's ingesting)
  • large files indication

There is still a minor issue with background flickering after validation is done; can't be fixed in the following issues.

PS.
This PR also includes some code cleaning:

  • removes some of unused code
  • unifies assets usage

@roll roll requested a review from romicolman November 12, 2024 16:03
Copy link

cloudflare-workers-and-pages bot commented Nov 12, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7d6ec12
Status: ✅  Deploy successful!
Preview URL: https://2c54f3b8.opendataeditor.pages.dev
Branch Preview URL: https://622-large-files.opendataeditor.pages.dev

View logs

Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

Thank you @roll for working on these changes. A couple of comments:

Can we change the "Validating..." text for "Checking errors..."?
Apart form that, everything looks ok. One small change, if possible. There seems to a lot of empty space with the introduction of the loading bar on the Upload screen. Can we adjust that a little bit? If this is not possible, merge it. It's not central to the implementation.

@roll
Copy link
Collaborator Author

roll commented Nov 13, 2024

Thanks!

Can we change the "Validating..." text for "Checking errors..."?

Done

Apart form that, everything looks ok. One small change, if possible. There seems to a lot of empty space with the introduction of the loading bar on the Upload screen. Can we adjust that a little bit?

I think it's something for @Faithkenny to test amd create a new issue if needed. There is a thing that without this empty space the size of the dialog will be "jumping" when there is loading/messaged/etc.

@romicolman
Copy link
Collaborator

OK @roll. Let's implement it as it is for now.

@roll
Copy link
Collaborator Author

roll commented Nov 18, 2024

@romicolman
Thanks! Please use the "Approve" so it can be merged

Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

This is approved :)

@roll
Copy link
Collaborator Author

roll commented Nov 18, 2024

Thanks!

@roll roll merged commit 021fc2e into main Nov 18, 2024
9 checks passed
@roll roll deleted the 622/large-files branch November 18, 2024 14:15
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.

Large files - Loading state
2 participants