-
Notifications
You must be signed in to change notification settings - Fork 43
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 ActionsColumn to Migration waves table #2109
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: sarinailinger <sn0533129699@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the shifted alignment, the action column changes LGTM
Signed-off-by: sarinailinger <sn0533129699@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: Maayan Hadasi <mguetta@redhat.com>
e93d196
to
782ff18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
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