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

Enable "Event Generating Elements Should Be Instrumented" ESLint rule for more O11y Apps #165647

Merged

Conversation

CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented Sep 5, 2023

Summary

This enables the ESLint rule that requires event generating UI elements to have a data-test-subj for more Observability apps.

Specifically:

  • observability_ai_assistant
  • observability_shared
  • observability_onboarding
  • profiling

In addition this also adds the EuiButtonIcon component as a component which needs a data-test-subj.

Effect

This PR will trigger the CI to create a commit that adds a data-test-subj value for these elements:

export const EVENT_GENERATING_ELEMENTS = [
  'EuiButton',
  'EuiButtonEmpty',
  'EuiButtonIcon',
  'EuiLink',
  'EuiFieldText',
  'EuiFieldSearch',
  'EuiFieldNumber',
  'EuiSelect',
  'EuiRadioGroup',
  'EuiTextArea',
];

to all plugins that the rule is active for, and don't have a value set yet.

Once the PR is merged, engineers working on all these plugins will see a warning in their IDE when no data-test-subj is set on any of the aforementioned elements. ESLint will also be able to 'fix' this by suggesting a value based on the plugin name, React component name, Eui element name and if present, the translated text content.

Elements that already have a data-test-subj will not be touched, and engineers can always adjust the suggestion that the rule autogenerates (either in your IDE or by commiting changes afterwards).

@CoenWarmer CoenWarmer requested a review from a team as a code owner September 5, 2023 08:13
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@CoenWarmer CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Sep 5, 2023
@CoenWarmer CoenWarmer changed the title Enable Event Generating Elements Should Be Instrumented rule for more… Enable "Event Generating Elements Should Be Instrumented" ESLint rule for more O11y Apps Sep 5, 2023
@kibanamachine kibanamachine requested review from a team as code owners September 5, 2023 08:45
@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Sep 5, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APM changes and Oblt onboarding changes LGTM

@CoenWarmer CoenWarmer force-pushed the feat/enable-eslint-rule-for-ai-assistant branch from 81829f4 to ca1f86e Compare September 5, 2023 10:13
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Profiling changes LGTM

…:CoenWarmer/kibana into feat/enable-eslint-rule-for-ai-assistant
@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 5, 2023

💔 Build Failed

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 3.7MB 3.7MB +503.0B
exploratoryView 200.7KB 200.9KB +211.0B
infra 2.0MB 2.0MB +1.1KB
observability 1.0MB 1.0MB +460.0B
observabilityAIAssistant 180.7KB 180.8KB +122.0B
observabilityOnboarding 293.6KB 295.0KB +1.4KB
profiling 335.9KB 337.7KB +1.8KB
synthetics 909.7KB 910.4KB +653.0B
ux 166.0KB 166.0KB +47.0B
total +6.3KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@CoenWarmer CoenWarmer enabled auto-merge (squash) September 5, 2023 12:09
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infra changes LGTM

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@CoenWarmer CoenWarmer merged commit 772bc0c into elastic:main Sep 5, 2023
@kibanamachine kibanamachine added v8.11.0 backport:skip This commit does not require backporting labels Sep 5, 2023
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.