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 for tables #419

Merged
merged 8 commits into from
Aug 9, 2023
Merged

feat: Loading for tables #419

merged 8 commits into from
Aug 9, 2023

Conversation

janmichek
Copy link
Collaborator

@janmichek janmichek commented Aug 2, 2023

Description

fixes #150

Loader style will be improved once delivered from Danilo

Demo

firefox_KnfzPwFNVM.mp4

Checklist:

@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Deployed to https://pr-419-aescan.stg.aepps.com

@lukeromanowicz
Copy link
Contributor

If you don't mind I'll review it once panel loading indicators are done since there is far less changes

@janmichek janmichek force-pushed the Loading-indicator-for-tables branch from 69cf1ec to 8e34f1a Compare August 7, 2023 08:22
@janmichek
Copy link
Collaborator Author

If you don't mind I'll review it once panel loading indicators are done since there is far less changes

I don't think this dependency is needed. Can be reviewed independently

Copy link
Contributor

@lukeromanowicz lukeromanowicz 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 working very nicely and apart from a minor refactoring proposition I'd accept it. My question is wether this is a concept showcase that will be applied to the remaining tables such as:

2023-08-08.09-54-28.mp4

or are you finished? In the second case I would add it in the other tables too :)

}

const paginatedContent = ref()
watch(
Copy link
Contributor

@lukeromanowicz lukeromanowicz Aug 8, 2023

Choose a reason for hiding this comment

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

I think we should make a dedicated composable e.g. useTableLoadingIndicator for that because this will be reused in most of the pages containing tables

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think now when I fixed the resetting it's not an issue anymore.
The pagination for tables is only handled by PaginatedContent component, right?
If I missed your point, please give me some examples of which other table would you reuse it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I misinterpreted the code. Please ignore it

@janmichek
Copy link
Collaborator Author

This is working very nicely and apart from a minor refactoring proposition I'd accept it. My question is wether this is a concept showcase that will be applied to the remaining tables such as:
2023-08-08.09-54-28.mp4

or are you finished? In the second case I would add it in the other tables too :)

You are right, I forgot to reset the store before navigating. Fixed

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

I found some issues during testing, could you please have a look at them?

  • Table content changes width when switching tabs
  • 500 error on page reload
Screen.Recording.2023-08-08.at.23.57.02.mov
  • Oracles events does not display the loader
Screenshot 2023-08-09 at 00 05 30

I will review again once fixed.

@janmichek
Copy link
Collaborator Author

I found some issues during testing, could you please have a look at them?

* Table content changes width when switching tabs

* 500 error on page reload

Screen.Recording.2023-08-08.at.23.57.02.mov

* Oracles events does not display the loader
Screenshot 2023-08-09 at 00 05 30

I will review again once fixed.

Thanks for the detailed check. All fixed. It produced one more conflict, I would better resolve in here
#95

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

From a UI / UX perspective it looks good. Maybe it can be improved further but that's a good base. Good job!

@janmichek
Copy link
Collaborator Author

From a UI / UX perspective it looks good. Maybe it can be improved further but that's a good base. Good job!

SHould I set some followup issue? What you desire to improve?

Copy link
Contributor

@lukeromanowicz lukeromanowicz 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 to me 👍 well done

}

const paginatedContent = ref()
watch(
Copy link
Contributor

Choose a reason for hiding this comment

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

I misinterpreted the code. Please ignore it

@janmichek janmichek merged commit 3c20573 into develop Aug 9, 2023
@janmichek janmichek deleted the Loading-indicator-for-tables branch August 9, 2023 11:10
@Liubov-crypto
Copy link
Collaborator

LGTM

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.

Loading indicator for tables
4 participants