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.

Sign up for GitHub

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

Jump to bottom

frontend: Add crd's categories #1940

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

frontend: Add crd's categories #1940

wants to merge 7 commits into from

Conversation

Guilamb
Copy link

@Guilamb Guilamb commented Apr 30, 2024

Description:

As mentioned in #955, this pull request addresses the need to filter CRDs by categories.
Here the features implemented:

  • List of Categories: Added the ability to list all available categories.
  • Category Selection: Implemented the functionality to select and unselect categories.
  • Filtering: Enabled filtering CRDs based on selected categories.

Implementation Details:

  • Introduced a new component, CRDCategoriesList, to facilitate category selection and filtering.
  • Utilized Redux for state management, specifically the filterSlice, to store and update the selected categories.
  • Integrated with existing CRD functionality to fetch and display categories dynamically.

Changes Made:

  • Added the CRDCategoriesList component to handle category selection and filtering.
  • Updated Redux state management to accommodate the new category filter.
  • Enhanced UI to display available categories and allow user interaction.

Feedback Request:

  • Any suggestions for optimizing loading time for the CRDCategoriesList component.
  • Review of Redux state management implementation and usage.
  • Feedback on UI/UX improvements or any potential issues.

Known Issues:

  • Currently experiencing loading time issues with the CRDCategoriesList component. Gather categories when initially loading the CRD may resolve the issue.

joaquimrocha
joaquimrocha requested changes May 3, 2024
View reviewed changes
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Hi @Guilamb . Thanks for the PR. I think adding the category filter and column makes a lot of sense for the CRDs. I think the approach and the code needs a few tweaks, though. So please check out my comments.
Please note that our search filters will change very soon and I think the category filter will be given for free after we have this PR merged. So after this new table PR, maybe all you need to do is add the new table column to that list.

frontend/src/components/common/Resource/ResourceTable.tsx Outdated Show resolved Hide resolved
frontend/src/components/common/Resource/ResourceTable.tsx Outdated Show resolved Hide resolved
frontend/src/components/crd/List.tsx Outdated Show resolved Hide resolved
frontend/src/lib/k8s/cluster.ts Outdated Show resolved Hide resolved
frontend/src/components/crd/Details.tsx Outdated Show resolved Hide resolved
frontend/src/components/common/CRDCategoriesList.tsx Outdated Show resolved Hide resolved
frontend/src/components/common/CRDCategoriesList.tsx Outdated Show resolved Hide resolved
frontend/src/components/common/SectionFilterHeader.tsx Outdated Show resolved Hide resolved
frontend/src/components/crd/List.tsx Outdated Show resolved Hide resolved
frontend/src/redux/filterSlice.ts Outdated
Comment on lines 11 to 12
/** The categories to filter on. */
categories: string[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Categories is something that belongs only to the CRDs, so I don't think we should make it global like this.

@joaquimrocha joaquimrocha marked this pull request as draft May 3, 2024 16:07
@ashu8912
Copy link
Member

@joaquimrocha Why was this turned to draft??? Is it still worked on?? @Guilamb

@Guilamb
Copy link
Author

Guilamb commented May 14, 2024

@joaquimrocha Why was this turned to draft??? Is it still worked on?? @Guilamb

Yes, I was out of the office the last 2 weeks. So I just saw the required changes.

@Guilamb
Copy link
Author

Guilamb commented May 14, 2024

Hi @Guilamb . Thanks for the PR. I think adding the category filter and column makes a lot of sense for the CRDs. I think the approach and the code needs a few tweaks, though. So please check out my comments. Please note that our search filters will change very soon and I think the category filter will be given for free after we have this PR merged. So after this new table PR, maybe all you need to do is add the new table column to that list.

Thank you for your feedback. Since the new search function resolves the issue of filtering CRDs by category (and possibly by namespace as well), should I remove the feature from the PR ?

@Guilamb Guilamb force-pushed the feature-filter-by-category branch from 07fbca8 to 6ea9925 Compare May 16, 2024 08:17
@Guilamb
Copy link
Author

Guilamb commented May 16, 2024

Since #1921 was merged, I removed the category filter and resolved this : #1940 (comment)

@Guilamb Guilamb changed the title frontend: Add crd's categories filter frontend: Add crd's categories May 17, 2024
@illume
Copy link
Contributor

illume commented May 17, 2024

Since #1921 was merged, I removed the category filter and resolved this : #1940 (comment)

Thanks @Guilamb

It's saying there's some conflicts now...

image

illume
illume reviewed May 17, 2024
View reviewed changes
frontend/src/redux/filterSlice.ts Outdated
}
},
removeCategoryFilter(state, action: PayloadAction<string>) {
console.log('removing : ', action.payload, 'from: ', state.categories.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this console.log.

frontend/src/redux/filterSlice.ts Outdated
removeCategoryFilter(state, action: PayloadAction<string>) {
console.log('removing : ', action.payload, 'from: ', state.categories.toString());
state.categories = state.categories.filter(category => category !== action.payload);
console.log('after removing : ', state.categories.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this console.log.

frontend/src/components/common/CRDCategoriesList.tsx Outdated Show resolved Hide resolved
illume
illume reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

I went through the existing comments and closed the conversation that looked like they had been addressed. Added a couple of notes about some console.log that were left in the code.

@Guilamb Guilamb force-pushed the feature-filter-by-category branch from e28d115 to f407dfe Compare May 21, 2024 10:38
@Guilamb
Resolve merge conflicts
cde9a58 
Signed-off-by: guilhane <[email protected]>
@Guilamb
Copy link
Author

Guilamb commented May 21, 2024

I've resolved merge conflicts and removed the remnant console.logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@illume illume illume left review comments

@joaquimrocha joaquimrocha joaquimrocha requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Guilamb @ashu8912 @illume @joaquimrocha