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 loading states #660

Merged
merged 15 commits into from
Dec 2, 2024
Merged

Proper loading states #660

merged 15 commits into from
Dec 2, 2024

Conversation

roll
Copy link
Collaborator

@roll roll commented Nov 26, 2024


@romicolman
Can you please try? It now properly handle states/errors for all the main operations (create/save/delete). It's better to test on large files.

@roll roll requested a review from romicolman November 26, 2024 15:57
Copy link

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

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: e3a7e6a
Status: ✅  Deploy successful!
Preview URL: https://1a17da6f.opendataeditor.pages.dev
Branch Preview URL: https://433-proper-loading-states.opendataeditor.pages.dev

View logs

@roll roll changed the title 433/proper loading states Proper loading states Nov 27, 2024
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.

Hi @roll! The implementation of loading states is OK.

Regarding the description on the Rename option, can you revert changes and keep the original version? (order and list of options - seee image below):

Captura de pantalla 2024-11-28 a la(s) 8 55 06 a  m

Reason behind this: we did not add description for rename because in this case, the category was transparent for users.

@roll
Copy link
Collaborator Author

roll commented Nov 28, 2024

@romicolman
Thanks.

Reverted the change, please review (without a description and with this inconsistent order of actions the menu feels unfinished to me but might still be good enough as you say).

@guergana
Copy link
Collaborator

Can we please merge this after: #654 ? these are three PRs with too many changes all based in main, i dont think it's a good idea. It's going to be very difficult with the merging

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.

Approved :)

@roll
Copy link
Collaborator Author

roll commented Dec 2, 2024

Thanks!

@roll roll merged commit 08a3014 into main Dec 2, 2024
9 checks passed
@roll roll deleted the 433/proper-loading-states branch December 2, 2024 14:24
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.

Rename description is missing Implement proper loading states
3 participants