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

Add filter to Policies and Roles tables, refactor filter #3690

Merged
merged 1 commit into from Aug 24, 2023

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented Aug 14, 2023

  • Moved repeatable filter code to Table.useOrdering Created Table.useFiltering
  • Converted the most of the Table to Typescript (splitted code to typed.tsx and untyped.js)
  • Moved <Filter /> to <Table.Filter /> and used it as one of toolbar actions
  • Added filter for list of attached policies, associated roles, buckets. Re-use Table.useFiltering for these components

Screenshot from 2023-08-15 16-28-26
Screenshot from 2023-08-15 16-30-20
Screenshot from 2023-08-15 16-30-47
Screenshot from 2023-08-15 16-31-01

  • Changelog entry (skip if change is not significant to end users, e.g. docs only)

@fiskus fiskus marked this pull request as draft August 14, 2023 10:38
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #3690 (ca64862) into master (ac96f3d) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head ca64862 differs from pull request most recent head f86962e. Consider uploading reports for the commit f86962e to get more accurate results

@@            Coverage Diff             @@
##           master    #3690      +/-   ##
==========================================
- Coverage   36.14%   36.14%   -0.01%     
==========================================
  Files         678      680       +2     
  Lines       29792    29778      -14     
  Branches     4348     4343       -5     
==========================================
- Hits        10769    10763       -6     
- Misses      17890    17895       +5     
+ Partials     1133     1120      -13     
Flag Coverage Δ
api-python 91.35% <ø> (ø)
catalog 9.80% <0.00%> (-0.03%) ⬇️
lambda 86.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
catalog/app/containers/Admin/Buckets/Buckets.tsx 0.00% <0.00%> (ø)
...tainers/Admin/RolesAndPolicies/AssociatedRoles.tsx 0.00% <0.00%> (ø)
...ainers/Admin/RolesAndPolicies/AttachedPolicies.tsx 0.00% <0.00%> (ø)
...ners/Admin/RolesAndPolicies/BucketsPermissions.tsx 0.00% <0.00%> (ø)
...g/app/containers/Admin/RolesAndPolicies/Filter.tsx 0.00% <0.00%> (ø)
...app/containers/Admin/RolesAndPolicies/Policies.tsx 0.00% <0.00%> (ø)
...og/app/containers/Admin/RolesAndPolicies/Roles.tsx 0.00% <0.00%> (ø)
catalog/app/containers/Admin/Table/Filter.tsx 0.00% <ø> (ø)
catalog/app/containers/Admin/Table/Table.tsx 0.00% <0.00%> (ø)
catalog/app/containers/Admin/Table/index.js 0.00% <0.00%> (ø)
... and 2 more

... and 40 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fiskus fiskus marked this pull request as ready for review August 15, 2023 13:25
@fiskus fiskus requested a review from nl0 August 18, 2023 11:40
Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

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

i see a fundamental design issue here -- filtering should not be an "extension" for ordering. instead, it should be a standalone composable unit (a hook in this case), which gets applied before ordering and its application result is be passed down to ordering as an input

@nl0
Copy link
Member

nl0 commented Aug 18, 2023

(splitted code to typed.tsx and untyped.js)

i'd suggest to rename typed.tsx into Table.tsx following the implicit convention observed throughout the codebase (keeping the untyped stuff in untyped.js until it's converted)

@fiskus fiskus force-pushed the admin-filters branch 2 times, most recently from 7515d59 to aa875ae Compare August 22, 2023 13:29
@fiskus
Copy link
Member Author

fiskus commented Aug 22, 2023

I rewrote the code, moving filter outside Table.useOrdering and Table.Toolbar

@fiskus fiskus requested a review from nl0 August 22, 2023 13:32
catalog/app/containers/Admin/Table/Table.tsx Outdated Show resolved Hide resolved
catalog/app/containers/Admin/Table/Table.tsx Outdated Show resolved Hide resolved
catalog/app/containers/Admin/Buckets/Buckets.tsx Outdated Show resolved Hide resolved
catalog/app/containers/Admin/Buckets/Buckets.tsx Outdated Show resolved Hide resolved
catalog/app/containers/Admin/Table/Table.tsx Outdated Show resolved Hide resolved
@fiskus fiskus enabled auto-merge August 23, 2023 13:37
@fiskus fiskus disabled auto-merge August 23, 2023 13:38
@fiskus fiskus enabled auto-merge August 23, 2023 13:52
@fiskus fiskus requested a review from nl0 August 23, 2023 13:52
nl0
nl0 previously approved these changes Aug 23, 2023
catalog/app/containers/Admin/Table/Table.tsx Outdated Show resolved Hide resolved
catalog/app/containers/Admin/Table/Table.tsx Outdated Show resolved Hide resolved
@fiskus
Copy link
Member Author

fiskus commented Aug 23, 2023

Resolved CHANGELOG conflict, added types, and squashed commits. I described new commits in comments

@fiskus fiskus enabled auto-merge August 23, 2023 16:32
@fiskus fiskus requested a review from nl0 August 23, 2023 16:34
@fiskus fiskus added this pull request to the merge queue Aug 24, 2023
Merged via the queue into master with commit fe48902 Aug 24, 2023
38 of 40 checks passed
@fiskus fiskus deleted the admin-filters branch August 24, 2023 15:12
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.

None yet

2 participants