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(ui/tasks): add pagination on tasks listing page #10293

Merged

Conversation

gaurav2733
Copy link
Contributor

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Apr 16, 2024
@gaurav2733 gaurav2733 marked this pull request as ready for review April 16, 2024 16:29
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

nice! in general I think we should avoid a pagination context since it obscures where the data is coming from and might end up being difficult to maintain

I think we can simplify things by handling pagination in the DataFlowJobsTab component itself and we can always write a new query to get tasks and paginate like we do elsewhere

import { useBaseEntity } from '../../EntityContext';
import { EntityType } from '../../../../../types.generated';
import { EntityList } from './components/EntityList';
import { useEntityRegistry } from '../../../../useEntityRegistry';
import { useTaskPagination } from './components/TaskPaginationContext';

export const DataFlowJobsTab = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if instead of needing to wrap the profile in a pagination provider, we just fetch the data from this component and do the pagination in this component here?

we could always default fetch the first 10 (instead of 100 like we do now) but then paginate from this component entirely like we do in most other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -70,7 +70,7 @@ query getDataFlow($urn: String!) {
downstream: lineage(input: { direction: DOWNSTREAM, start: 0, count: 100 }) {
...partialLineageResults
}
childJobs: relationships(input: { types: ["IsPartOf"], direction: INCOMING, start: 0, count: 100 }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could then write another query like getDataFlowChildJobs that takes in a start and count and use that when we want to paginate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 71 to 74
totalJobs,
pageSize,
lastResultIndex,
showTaskPagination = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

totalJobs and showTaskPagination are a little too specific here - let's rename to totalAssets and showPagination to make this generic so we can use pagination in EntityList everywhere if we desire.

we then will probably need to pass in page and setPage from the parent so pagination can be handled outside of here and we don't have to rely on a pagination context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@gaurav2733 gaurav2733 marked this pull request as ready for review April 18, 2024 12:27
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

looking good! thanks for making this change

i have one request that would be great if you could do in a followup

@@ -125,6 +125,53 @@ query getDataFlow($urn: String!) {
}
}

query getDataFlowChildJobs($urn: String!, $start: Int, $count: Int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice! as a followup we can actually remove the childJobs that we're currently getting in the main dataflow query since we're getting it now when you open the tab

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

actually i'm going to ask for a couple of small UI edits before merging this:

can we update the styling of the pagination at the bottom of the page to look like it does elsewhere? right now the pages are on the far right when they should be centered. also, the pagination bar should be fixed to the bottom of the screen and allow the contents to scroll within it. this is what i'm seeing:
image

an example of how it looks elsewhere like on the domains contents page:
image

once these two quick things are dealt with we're ready to roll

@gaurav2733
Copy link
Contributor Author

@chriscollins3456 here is the attached video
Screencast from 19-04-24 09:00:44 AM IST.webm

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

nice looks good!

@chriscollins3456 chriscollins3456 merged commit d7cd1fc into datahub-project:master Apr 19, 2024
37 of 38 checks passed
sleeperdeep pushed a commit to sleeperdeep/datahub that referenced this pull request Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants