Fixes #39397 - Update AnsibleRolesSwitcher components to use DualList#792
Conversation
kmalyjur
left a comment
kmalyjur
left a comment
There was a problem hiding this comment.
It works well. I just have a few points:
AnsibleRolesSwitcherError.jsstill uses PF3 https://github.com/pondrejk/foreman_ansible/blob/09585aec0784b223f63bafaa84fa72886cd40480/webpack/components/AnsibleRolesSwitcher/components/AnsibleRolesSwitcherError.js#L2- Missing space between the number and the name
| li { | ||
| padding: 0.5rem 0.75rem; | ||
| margin-bottom: 0.25rem; | ||
| background-color: $color-pf-black-200; |
There was a problem hiding this comment.
Should be: background-color: $color-pf-black-200;
There was a problem hiding this comment.
I'm so sorry, it should have been this for lines 33-34:
background-color: var(--pf-v5-global--BackgroundColor--200);
color: var(--pf-v5-global--Color--200);
| toDestroyRoles, | ||
| error, | ||
| } = this.props; | ||
| }, []); |
There was a problem hiding this comment.
Missing dependencies warning. eslint-disable could be added.
| : `id !^ (${assignedRoles.map(role => role.id).join(', ')})`; | ||
| return { search: searchString }; | ||
| }; | ||
| export const ALL_ROLES_PER_PAGE = 1000; |
There was a problem hiding this comment.
Do we have some backstory why to use this number? Eventually users with more than 1000 roles will have issues.
There was a problem hiding this comment.
eh, not really, it's arbitrary. I have a card for adjusting DualList itself, so I could probably include some form of pagination there to solve this. For this PR, I could increase this or use 'all' to fetch everything, wdyt?
@kmalyjur addressed, thanks! |
3 participants
Overview of Changes
Note that ordering in DualList is currently not working, it will be addressed separeately
Roles tab with the proposed changes:
with inheritance: