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

[#187] [On Week] Improve and enrich EUI ESLint plugin #8304

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

weronikaolejniczak
Copy link
Contributor

@weronikaolejniczak weronikaolejniczak commented Feb 3, 2025

Summary

Closes #187

This PR is part of an On Week initiative to improve the sate of the EUI ESLint plugin and enrich with useful rules.

The changes done:

  • migrated the package to ESM and TypeScript
  • added no-restricted-eui-imports rule to display a warning on JSON token imports
  • moved no-css-color and prefer-css-attribute-for-eui-components from kbn-eslint-plugin-css
    • no-css-color rule: highlights hard-coded colors
    • prefer-css-attribute-for-eui-components rule: recommends css use over style (inline styles)

Potential future changes:

  • migrate to Vitest (RuleTester) for out-of-the-box ESM and TS support, and superior performance to Jest
  • create a CI job to run the tests for when the package has been changed
  • implement design tokens specific rules

no-restricted-eui-imports

I added a new rule to our ESLint plugin, no-restricted-eui-imports, that won't conflict with the inbuilt no-restricted-imports rule and will allow us to define several error levels at once. I.e. we want some imports to be marked as warning and not an error, as it's done in Kibana.

Screenshot 2025-01-07 at 11 48 02 Screenshot 2025-01-07 at 11 50 41

Context:

The JSON token imports in @kbn/eslint/module_migration need to be removed as well:

https://github.com/elastic/kibana/blob/212b1926743ca5821992c2877d9c68f621e1875e/packages/kbn-eslint-config/.eslintrc.js#L131-L140

no-css-color and prefer-css-attribute-for-eui-components from kbn-eslint-plugin-css

Written by @eokoneyo. We agreed to move the rules to EUI repo, as the rules concern EUI components and may benefit other EUI consumers like Cloud UI.

Example files in Kibana:

  • src/platform/plugins/shared/kibana_react/public/page_template/no_data_page/no_data_card/elastic_agent_card.tsx

Screenshot 2025-02-28 at 17 00 38

  • x-pack/examples/alerting_example/public/alert_types/always_firing.tsx

Screenshot 2025-02-28 at 17 01 21

With additional configuration:

  • src/platform/plugins/private/vis_types/vega/public/data_model/utils.ts

Screenshot 2025-02-28 at 17 01 55

Useful resources

QA

  1. Follow the steps on this PR to test in Kibana.
  2. Run the tests: yarn workspace @elastic/eslint-plugin-eui test.

@weronikaolejniczak weronikaolejniczak changed the title Chore/187 eslint plugin eui [#187] [On Week] Improve and enrich EUI ESLint plugin Feb 3, 2025
@weronikaolejniczak weronikaolejniczak force-pushed the chore/187-eslint-plugin-eui branch from d69e9c5 to 7fc8386 Compare February 3, 2025 13:53
@weronikaolejniczak weronikaolejniczak force-pushed the chore/187-eslint-plugin-eui branch from 5248aa8 to 93831a6 Compare February 7, 2025 11:44
@weronikaolejniczak weronikaolejniczak self-assigned this Feb 7, 2025
@weronikaolejniczak weronikaolejniczak linked an issue Feb 25, 2025 that may be closed by this pull request
@weronikaolejniczak weronikaolejniczak force-pushed the chore/187-eslint-plugin-eui branch from b8f4827 to 7a40fae Compare February 26, 2025 13:28
@weronikaolejniczak weronikaolejniczak marked this pull request as ready for review March 3, 2025 12:17
@weronikaolejniczak weronikaolejniczak requested a review from a team as a code owner March 3, 2025 12:17
@weronikaolejniczak weronikaolejniczak removed their assignment Mar 7, 2025
@weronikaolejniczak weronikaolejniczak self-assigned this Mar 18, 2025
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @weronikaolejniczak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate euiThemeVars
4 participants