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/tokens): delete multiple tokens #2424

Merged
merged 7 commits into from
Nov 22, 2023

Conversation

erka
Copy link
Collaborator

@erka erka commented Nov 21, 2023

Refs: #2275, FLI-215

@erka erka force-pushed the delete-multiple-tokens branch from 682c036 to bd271c0 Compare November 21, 2023 17:34
- implemented the logic for multiple tokens deletion
- some ui changes and cleanup
@erka erka force-pushed the delete-multiple-tokens branch from 544791c to eea794e Compare November 21, 2023 19:08
@erka
Copy link
Collaborator Author

erka commented Nov 21, 2023

@markphelps please take a look and provide feedback.

The delete button is too big as the replacement of 'Name'. Any workaround for it?
The delete token modal probably should better formatting for multiple tokens. Any suggestions?
Anything else?

@markphelps
Copy link
Collaborator

@erka this looks great! I responded to most the questions in the PR on your PR erka#1

I think its ok that the Delete button hides the name field on the table

Really appreciate this!! Works great!

@erka erka force-pushed the delete-multiple-tokens branch from 6a27e3a to eea794e Compare November 21, 2023 23:23
feat: updates for delete multiple tokens PR
@erka
Copy link
Collaborator Author

erka commented Nov 21, 2023

There is some side effect which happens right now. After deletions the tokens if you create the new one it will be selected for deletion. I believe the rowSelection state isn't cleanup up after the api calls. If the rowSelection will be move to the Tokens.tsx but then you need RowSelectionState there which looks bad.

By the way there is one strange state with filter too in main branch. If you use filter to find one token and delete it you will see a blank table and no filter input... It looks like as a bug.

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b054623) 70.76% compared to head (f7b40cf) 70.76%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2424   +/-   ##
=======================================
  Coverage   70.76%   70.76%           
=======================================
  Files          81       81           
  Lines        8110     8110           
=======================================
  Hits         5739     5739           
  Misses       2025     2025           
  Partials      346      346           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@markphelps
Copy link
Collaborator

@erka what do you think about erka#2 , which passes down the existing tokensVersion prop to then reset selected rows if the underlying tokens change (such as delete)?

I think it might be nicer than using events?

By the way there is one strange state with filter too in main branch. If you use filter to find one token and delete it you will see a blank table and no filter input... It looks like as a bug.

Yes ☝🏻 def sounds like a bug. We should file and fix that. I'm happy to file it in the morning (US) or if you are able to first, either way.

Thank you again for this contribution!!

@erka erka marked this pull request as ready for review November 22, 2023 08:57
@erka erka requested a review from a team as a code owner November 22, 2023 08:57
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

thank you for the contribution @erka !! Looks great and very much appreciated!

@markphelps
Copy link
Collaborator

@all-contributors please add @erka for code

Copy link
Contributor

@markphelps

I've put up a pull request to add @erka! 🎉

@markphelps markphelps enabled auto-merge (squash) November 22, 2023 12:56
@markphelps markphelps merged commit 8a72265 into flipt-io:main Nov 22, 2023
28 of 29 checks passed
@erka erka deleted the delete-multiple-tokens branch November 22, 2023 16:44
@markphelps
Copy link
Collaborator

Will create a release later today with this

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.

2 participants