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: migrating transfer history page to api wrapper #351

Merged

Conversation

AnurosePrakash
Copy link

@AnurosePrakash AnurosePrakash commented Jul 12, 2023

Pull Request

Migrating transfer history page to api wrapper

How Has This Been Tested?

Mock backend

PR is blocked by

Checklist

  • I have formatted the title correctly and precisely
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas and public classes/methods
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings (performed checkstyle check locally)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have added/updated copyright headers

Copy link

@SaadEGI SaadEGI left a comment

Choose a reason for hiding this comment

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

Request changes

@AnurosePrakash AnurosePrakash self-assigned this Jul 19, 2023
SaadEGI
SaadEGI previously approved these changes Jul 19, 2023
AnurosePrakash and others added 5 commits August 11, 2023 18:00
…_wrapper

# Conflicts:
#	src/app/core/services/api/edc-api.service.ts
#	src/app/routes/connector-ui/dashboard-page/dashboard-page/dashboard-page-data.service.ts
…nsfer_history_page_migrations_to_api_wrapper
…tions_to_api_wrapper' into feat/transfer_history_page_migrations_to_api_wrapper
Copy link
Collaborator

@richardtreier richardtreier left a comment

Choose a reason for hiding this comment

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

image

  • On initial load the refresh button should not show
  • The refresh button uses raised buttons + icons, which isn't used anywhere else anymore, it should be a flat button.
  • Maybe instead of offering a refresh button, we should auto update the list every 5s.
  • The pagination should be on the right as in the other pages
  • Both Detail Dialogs have broken Layouts
  • The direction icons look unbalanced with paddings and positioning
  • The status icons look unbalanced wwith paddings and positioning
  • Last Update could be the 2nd columns maybe
  • Table column sizes could have further adjustments. Columns that we know that won't grow large could be small (e.g. date, icon, status, details)

@AnurosePrakash AnurosePrakash marked this pull request as draft August 14, 2023 11:36
@AnurosePrakash AnurosePrakash marked this pull request as ready for review August 14, 2023 14:21
Copy link
Collaborator

@richardtreier richardtreier left a comment

Choose a reason for hiding this comment

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

Looks good. TODO:

  • Fix MDS Link color
  • Add "open_in_new" icon for assets where available

Copy link
Collaborator

@richardtreier richardtreier left a comment

Choose a reason for hiding this comment

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

LGTM!

@richardtreier richardtreier merged commit b15b648 into main Aug 14, 2023
9 checks passed
@richardtreier richardtreier deleted the feat/transfer_history_page_migrations_to_api_wrapper branch August 14, 2023 15:28
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.

Migrate Transfer History Page to API Wrapper
3 participants