-
Notifications
You must be signed in to change notification settings - Fork 40
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 <[email protected]>
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)
</DropdownItem> | ||
<ActionsColumn | ||
items={[ | ||
// { |
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.
Please remove commented code.
// }, | ||
{ | ||
isAriaDisabled: | ||
migrationWave.applications.length === 0, |
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.
You use several ways of checking that the applications list is empty - they all OK but for the sake of consistency I would pick one option. My preference would be the one that is already used in the legacy code but feel free to chose.
what: t("terms.applications").toLowerCase(), | ||
}), | ||
onClick: () => { | ||
if (migrationWave.applications.length > 0) { |
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.
Do we need this check ? I would expect that isAriaDisabled === true
should block clicks.
// onClick: () => setWaveModalState(migrationWave), | ||
// }, | ||
{ | ||
isAriaDisabled: |
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.
nice! I haven't used it yet but isAriaDisabled
does bring some advantages over isDisabled
{ | ||
isAriaDisabled: | ||
migrationWave.applications?.length < 1 || | ||
!hasExportableApplications( |
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.
do we need to check for both conditions? I haven't tested this flow but looking at the code: when there is no apps then there should also be no exportable apps so the second condition alone would do the job.
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