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: filtering of recipe table #74

Merged
merged 8 commits into from
Apr 26, 2024

Conversation

lechnerc77
Copy link
Contributor

@lechnerc77 lechnerc77 commented Apr 16, 2024

This PR introduces filtering and sorting for the recipe table.

This covers parts of the rquirements from issue #32. The resource table is not yet enabled for sorting and filtering. This requires a bit more work due to the setup of the table.

AB#12039

Signed-off-by: Christian Lechner <lechnerc77@users.noreply.github.com>
Signed-off-by: Christian Lechner <lechnerc77@users.noreply.github.com>
@willtsai
Copy link
Contributor

Thanks @lechnerc77! We'll take a look at your PR today.
cc. @nithyatsu

@nithyatsu
Copy link
Contributor

Thank you so much @lechnerc77, these changes look good. Could you please add a screen shot of the new UI capturing the change?

@lechnerc77
Copy link
Contributor Author

lechnerc77 commented Apr 23, 2024

@nithyatsu sure I can add screenshots:

Sorting of table

TableSort

Table filter via filter area

TableFilter1

Table Filter via toolbar

TableFilter2

And when looking at the screenshots, there seems to be glitch in the toolbar that leads to a weird padding of the filter section and the title which is cut off. The toolbar is not really configurable, but the padding should imho work out of the box or am I missing something?

@nithyatsu
Copy link
Contributor

@lechnerc77 thank you so much for the screenshots, they look great! I will take a look at the code tomorrow to understand the overlap issue with the text and get back to you.

return (
<Table
title={title || 'Recipes'}
options={{ search: false, paging: false }}
options={{ search: true, paging: false, sorting: true }}
Copy link
Contributor

Choose a reason for hiding this comment

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

since we have the TableFilter, we can turn of the search so that the toolbar disappears.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the search. I changed the filtering option for Name to multi-select to be consistent with the other options

@nithyatsu
Copy link
Contributor

I did not find a way to get the overlapping text issue solved. But since we are now dropping the toolbar and the title, we should be good.

type: 'multiple-select',
},
];

return (
<Table
title={title || 'Recipes'}
Copy link
Contributor

@nithyatsu nithyatsu Apr 24, 2024

Choose a reason for hiding this comment

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

The title is redundant since we have the page title set already. Therefore we can remove this. I will make sure to remove the redundant title from a couple of other pages too.

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

@lechnerc77
Copy link
Contributor Author

I did not find a way to get the overlapping text issue solved. But since we are now dropping the toolbar and the title, we should be good.

Good to hear that I did not overlook something here!

Signed-off-by: Christian Lechner <lechnerc77@users.noreply.github.com>
Signed-off-by: Christian Lechner <lechnerc77@users.noreply.github.com>
Signed-off-by: Christian Lechner <lechnerc77@users.noreply.github.com>
Copy link
Contributor

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

Thanks @lechnerc77

@rynowak rynowak merged commit fa87fda into radius-project:main Apr 26, 2024
3 checks passed
@lechnerc77 lechnerc77 deleted the issue-32-recipe branch April 27, 2024 08:03
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.

4 participants