-
Notifications
You must be signed in to change notification settings - Fork 2
1267 listing piplines #1403
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
1267 listing piplines #1403
Conversation
…ave-gitops-enterprise into 1267-listing-piplines
…erprise into 1267-listing-piplines
…/weave-gitops-enterprise into 1267-listing-piplines
…ave-gitops-enterprise into 1267-listing-piplines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers for me. Some of the larger styling changes make me a tad nervous though.
ui-cra/src/App.tsx
Outdated
@@ -57,7 +59,7 @@ const GlobalStyle = createGlobalStyle` | |||
display: flex; | |||
flex-direction: column; | |||
margin: 0; | |||
|
|||
overflow:hidden; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially a larger change. Was something scrolling that should not have been?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, this should be removed .
marginLeft: small, | ||
paddingRight: medium, | ||
paddingLeft: medium, | ||
borderRadius: xs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit worried here. I wouldn't expect this needed to be changed. Can you elaborate @TheGostKasper ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ContentWrapper loading={isLoading} errorMessage={error?.message}> | ||
{data?.pipelines && ( | ||
<TableWrapper id="pipelines-list"> | ||
<FilterableTable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a URLAddressableTable
here? That would allow the user to create bookmarks. I think that is already exported from Core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know anything about URLAddressableTable
🤔 and i can't find it in Core, Can you walk me through it ?
); | ||
}; | ||
|
||
export const useCountPipelines = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may be able to compose useCountPipelines
from useListPipelines
:
export const useListPipelines = () => {
const pipelinsService = usePipelines();
return useQuery<ListPipelinesResponse, Error>(
[LIST_PIPLINES_KEY],
() => pipelinsService.ListPipelines({}),
{ retry: false },
);
};
export const useCountPipelines() {
const { data } = useListPipelines()
return data?.pipelines.length
}
We could also use a selector pattern as well, but composing here will reduce some duplication.
It fixes #1276
Show list of Pipelines
[Test coverage]
getTableInfo
sortTableByColumn
searchTableByValue