✨ add ActionsColumn to Migration waves table #2109
Conversation
Signed-off-by: sarinailinger <sn0533129699@gmail.com>
sarinailinger
requested review from
ibolton336,
sjd78 and
rszwajko
as code owners
sarinailinger
changed the title
There was a problem hiding this comment.
There are few changes in this PR. You could actually split it into 3 small PRs.
We can continue in one PR but please describe the changes:
- adjust the title to better reflect the changes (the more changes the harder to find good words :) )
- list changes in the PR description (first post) here are some examples: 🐛 Fix fetching in InfiniteScroller #2085 or 📖 WSL/Windows setup docs, rework setup docs #2080
I had also few comments/questions to the code (see below)
Signed-off-by: sari weinberger <sn0533129699@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2109 +/- ##
==========================================
+ Coverage 39.20% 41.98% +2.78%
==========================================
Files 146 175 +29
Lines 4857 5630 +773
Branches 1164 1394 +230
==========================================
+ Hits 1904 2364 +460
- Misses 2939 3250 +311
- Partials 14 16 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: sarinailinger <sn0533129699@gmail.com>
There was a problem hiding this comment.
Thank you for improving the diff and sorry for requesting changes in the code that you did not touch. I got tricked by the diff while reviewing in the browser. Fortunately @sjd78 has noticed that.
Looking at the new version I have some comments but the code worked for me and it looks good.
| @@ -388,7 +390,7 @@ export const MigrationWaves: React.FC = () => { | |||
| } | |||
| numRenderedColumns={numRenderedColumns} | |||
| > | |||
| {currentPageItems?.map((migrationWave, rowIndex) => ( | |||
| {currentPageItems?.map((migrationWave, rowIndex) => { | |||
There was a problem hiding this comment.
We are using implicit return now which may cause problems in some scenarios - compare:
()=> {<Foo/>;}
// vs
() => <Foo/>
if the component Foo takes multiple lines then it's wrapped in extra braces (())
| </Td> | ||
| <Td isActionCell id="row-actions"> | ||
| <ActionsColumn | ||
| open={isRowDropdownOpen === migrationWave.id} |
There was a problem hiding this comment.
The open prop is not needed here. Please also remove the isRowDropdownOpen flag and setter (line 89) as they are dead code now.
the same actions that there is currently should stay
but delete should be red & edit should be in another button(not in the list)
implement it by ActionsColumn