Skip to content
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 rowProps to saved objects management table #59758

Closed
bhavyarm opened this issue Mar 10, 2020 · 12 comments · Fixed by #60226
Closed

Add rowProps to saved objects management table #59758

bhavyarm opened this issue Mar 10, 2020 · 12 comments · Fixed by #60226
Assignees
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@bhavyarm
Copy link
Contributor

Describe the feature:
While trying to add a11y tests - we found out that rowProps are needed to be added to saved objects management table so that we can compute data-test-subj per rows easily

File to which rowProps need to be added: kibana/src/legacy/core_plugins/kibana/public/management/sections/objects/components/objects_table/components/table/table.js

Here is an example:
kibana/x-pack/legacy/plugins/ml/public/application/data_frame_analytics/pages/analytics_management/components/analytics_list/analytics_list.tsx

@bhavyarm bhavyarm added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Mar 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet
Copy link
Contributor

Should tackle that beginning of next week.

@bhavyarm
Copy link
Contributor Author

cc @LeeDr

@LeeDr
Copy link

LeeDr commented Mar 12, 2020

@pgayvallet I created this draft PR as a work-around #59968

But you can see that it's not as simple as if there were data-test-subj's on the fields, and specifically if the had the saved object title in them. But spaces and special characters could cause problems.

@pgayvallet
Copy link
Contributor

@LeeDr I'm afraid due to the range of characters allowed in the name, we won't be able to use it as a technical identifier of any sort. We are going to need to use the ID as identifier here if we want to do anything.

@pgayvallet
Copy link
Contributor

I just created #60226. However what we can do with the EUI tables configuration is quite limited:

  • I can only use dynamic values for data-test-subj on the root table's rowProps property.
  • Per row data-test-subj can only be static and can't depends on the displayed object, meaning that we can't inject IDs there
  • data-test-subj on the actions special column does not work, so I did not add it
  • data-test-subj on the individual actions does not work, so I did not add them

In conclusion, not sure all this is going to help you a lot, and I feel like the helper FTR pageObject methods are going to be required...

Please tell me if you see anything I should/can add to the PR

@bhavyarm
Copy link
Contributor Author

@dmlemeshko @LeeDr can I please get your inputs on this? Thanks!

@cchaos
Copy link
Contributor

cchaos commented Mar 16, 2020

Looking at the actual code for the actions column of Saved Objects, @pgayvallet is correct that there is no way to add a dynamic data-test-subj to each action item the way that the particular table is being created.

actions: [
{
name: i18n.translate(
'kbn.management.objects.objectsTable.table.columnActions.inspectActionName',
{ defaultMessage: 'Inspect' }
),
description: i18n.translate(
'kbn.management.objects.objectsTable.table.columnActions.inspectActionDescription',
{ defaultMessage: 'Inspect this saved object' }
),
type: 'icon',
icon: 'inspect',
onClick: object => goInspectObject(object),
available: object => !!object.meta.editUrl,
},
{

This passes the DefaultItemAction object which allows you to pass a data-test-subj but it can only be a fixed string for all rows. There's a couple actions you can take because of this limitation:

  1. Convert the actions column to use the custom render option which then you would have access to add properties directly to the rendered elements
  2. Wait until this EUI issue is addressed, released, and merged into Kibana [EuiBasicTable] Default action items should allow for dynamic names & descriptions eui#1401 Which you know could take a while.
  3. Apply a fixed string, but add a dynamic version elsewhere to help target it.

Sorry that EUI is lacking there at the moment, but you do have a couple paths for solutions.

@pgayvallet
Copy link
Contributor

@cchaos thanks for taking a look.

Apply a fixed string, but add a dynamic version elsewhere to help target it.

The 'special' actions row property seems to aggregate the actions under a dropdown under certain conditions (sometime the actions are inlined in the cell, sometime there is a dropdown button instead, did not really find what was triggering that. width? number of actions?). It seems that when in dropdown mode, even the static data-test-subj is not applied. Can you confirm (or infirm) that?

Convert the actions column to use the custom render option which then you would have access to add properties directly to the rendered elements

We are currently in the process of migrating this part to NP and TS, this is something I would really like to avoid in the very short term. If we could get the actions to have a static testSubj, this probably would be sufficient for now as the row/tr now have a dynamic one, which should be enough to target a specific action

@cchaos
Copy link
Contributor

cchaos commented Mar 17, 2020

The table will automatically combine action buttons in to a popover if there are more than 2. This is so there aren't a lot of text-less icon buttons in a row. I can confirm that passing data-test-subj to these buttons will apply them whether inline or in the popover.

Screen Shot on 2020-03-17 at 08:54:19

There is also a static dts for the popover trigger button called euiCollapsedItemActionsButton.

@pgayvallet
Copy link
Contributor

I can confirm that passing data-test-subj to these buttons will apply them whether inline or in the popover.

I don't know how I did not get that working. It's indeed working, thanks

@bhavyarm
Copy link
Contributor Author

Thanks for excellent work on this Pierre.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants