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

feat: loading states for modal buttons #2268

Merged
merged 8 commits into from
Apr 20, 2022

Conversation

kyteinsky
Copy link
Contributor

Description

Adds loading state to buttons in different modals.

@kyteinsky
Copy link
Contributor Author

SmartConfirmModal would need modifications if delete functionality also needs loading.

AndrewBastin
AndrewBastin previously approved these changes Apr 16, 2022
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

lgtm 💯

ask and wait for @liyasthomas to know whether we should do anything about the Delete option.

Although, I would think it will not hurt to do that.

@liyasthomas
Copy link
Member

@kyteinsky it would be great if we can introduce loading state in delete confirmation modals too. Mainly the delete request, delete collection, delete folder prompts on teams.

@kyteinsky
Copy link
Contributor Author

@liyasthomas There are two ways to add loading in delete confirmation modals, one by introducing an optional prop in SmartConfirmModal and the other by creating a new component altogether. Which path should I take?

@liyasthomas
Copy link
Member

Introduce a prop in SmartConfirmModal will be the best way to do it.

@kyteinsky
Copy link
Contributor Author

Should I change the Folder delete logic in the respective files CollectionMyFolder and CollectionTeamsFolder or move them to index.vue? Or maybe move the delete functions for Request and Collection to their respective files?

@AndrewBastin
Copy link
Member

@kyteinsky you can maintain whatever way it is right now and not worry about where the logic is (although, all of them should be in index.vue). We are planning a major rewrite of the collection tree structure and we will clear most of it during that time.

Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

lgtm 💯

@AndrewBastin AndrewBastin merged commit 2452b1b into hoppscotch:main Apr 20, 2022
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.

None yet

3 participants