[Enterprise Search] Refactor RoleMappingsTable to use EuiInMemoryTable #101918
Conversation
Both tables use the same actions
scottybollinger
added
auto-backport
scottybollinger
force-pushed
the
scottybollinger/table-refactor
branch
from
d74459e
to
5b0f023
Compare
This is way better than the bespoke one I wrote and it comes with pagination for free - Also fixes a typo in the i18n id
scottybollinger
force-pushed
the
scottybollinger/table-refactor
branch
from
5b0f023
to
78fb896
Compare
| incremental: true, | ||
| fullWidth: false, | ||
| placeholder: FILTER_ROLE_MAPPINGS_PLACEHOLDER, | ||
| 'data-test-subj': 'RoleMappingsTableSearchInput', |
There was a problem hiding this comment.
Are you using this attribute?
There was a problem hiding this comment.
Not yet, but we hope to write some Cypress tests before 8.0 and I believe it could be used there.
| const actionsCol: EuiBasicTableColumn<SharedRoleMapping> = { | ||
| field: 'id', | ||
| name: '', | ||
| align: 'right', | ||
| render: (_, { id }: SharedRoleMapping) => ( | ||
| <UsersAndRolesRowActions | ||
| onManageClick={() => initializeRoleMapping(id)} | ||
| onDeleteClick={() => handleDeleteMapping(id)} | ||
| /> | ||
| </> | ||
| ); | ||
| ), | ||
| }; |
There was a problem hiding this comment.
I feel like this should probably be an action column https://elastic.github.io/eui/#/tabular-content/tables#adding-actions-to-table but its fine if this got polished later
There was a problem hiding this comment.
Yeah, I went back and forth with it. Ultimately, I decided against it because it limits the display to 2 icon buttons before collapsing the others with an ellipsis and the forthcoming invitations table for pending invitations has 3. I wasn't sure if there was any design difference between bespoke actions and that actions column but didn't want to have a mixture of them on this layout if there was.
There was a problem hiding this comment.
Sorry for the lateness on this, but semi related to this, FYI we do need to add a column header to action columns - it's an a11y issue / axe failure for a table cell not to have an accompanying table heading. We can definitely add it later though, I intend to do an axe pass on our plugin next week
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
elastic#101918) * Add shared actions component Both tables use the same actions * Refactor RoleMappingsTable to use EuiInMemoryTable This is way better than the bespoke one I wrote and it comes with pagination for free - Also fixes a typo in the i18n id
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
#101918) (#101940) * Add shared actions component Both tables use the same actions * Refactor RoleMappingsTable to use EuiInMemoryTable This is way better than the bespoke one I wrote and it comes with pagination for free - Also fixes a typo in the i18n id Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>
Summary
EuiInMemoryTableis way better than the bespoke one I wrote and it comes with pagination for free. Will also use this component in the Users table coming in a future PR.Also adds a shared
UsersAndRolesRowActionscomponent for use in both tables.Please review
ent-searchcompanion PR as a part of this review.Checklist