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

[eslint] Custom lint rule to find non-localized text in JSX #5399

Closed
wants to merge 8 commits into from

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Nov 22, 2021

Summary

Caroline mentioned adding an item to our PR checklist to help catch hard-coded text in our components. I was curious if we could instead enforce this with a lint rule, and this is where I ended up. I disabled the check for jest & cypress tests.

This rule is not comprehensive! There are ways to bypass its logic, some of which are known but I'm sure there are many that are not. For example, you can do something like <MyComponent customLabel="something" /> and it will be allowed as only children are verified, and inside MyComponent the rule will see customLabel comes from a prop and assume it's safe.

The rule has flagged 44 existing problems in the codebase across 29 files:

Needs to use i18n

src/components/combo_box/combo_box_input/combo_box_input.tsx

  • line 216 removeOptionMessageContent has hard-coded text

src/components/datagrid/body/header/data_grid_header_cell.tsx

  • line 128 sortString conditional should be explicit, sortString != null
  • line 130 sortString has hard-coded text

src/components/date_picker/super_date_picker/date_popover/absolute_tab.tsx

  • line 131 sentenceCasedPosition is technically safe to use, but our sentence-casing strategy is an enemy to localization
  • line 131 has hard-coded text

src/components/date_picker/super_date_picker/date_popover/date_popover_button.tsx

  • line 107 formatTimeString needs localization work, and then be marked safe

src/components/date_picker/super_date_picker/date_popover/date_popover_content.tsx

src/components/date_picker/super_date_picker/quick_select_popover/recently_used.tsx

  • line 48 prettyDuration needs localization work, and then be marked safe

src/components/date_picker/super_date_picker/super_date_picker.tsx

  • line 381 prettyInterval needs localization work, and then be marked safe
  • line 408 prettyDuration needs localization work, and then be marked safe

src/components/markdown_editor/markdown_editor_footer.tsx

  • line 241 syntax prefix/link/suffix should be refactored to use a single localization token
  • line 291 syntax description/link should be refactored to use a single localization token

src/components/markdown_editor/plugins/markdown_tooltip/plugin.tsx

  • line 25 anchor & tooltip text need to be localized

src/components/notification/notification_event_meta.tsx

  • line 116 template literal should be localized

src/components/search_bar/filters/field_value_selection_filter.tsx

src/components/stat/stat.tsx

  • line 151 template literal that is probably better as a localization token; I feel this file may be a strong candidate for an accessibility audit

src/components/table/table_pagination/table_pagination.tsx

  • line 84 the : and itemsPerPage shoul dmove into the rowsPerPage i18n usage

Safe code that is flagged

These cases are okay and we need to find a way to ignore the error, my ideas are (in order of personal preference):

  • configuration option to allowlist functions/identifiers
  • in-code method to identify safe usages (e.g. decorators)
  • eslint-disable-line

src/components/avatar/avatar.tsx

  • line 165 calculatedInitials is a string computed by a function

src/components/basic_table/basic_table.tsx

  • line 726 captionElement can be the tableCaption from props

src/components/datagrid/body/popover_utils.tsx

  • line 39 we don't control the value of formattedText

src/components/expression/expression.tsx

  • line 160 lint rule needs updating to allow white-space string literals

src/components/form/form_row/form_row.tsx

  • line 248 lint rule needs updating to allow white-space string literals

src/components/highlight/highlight.tsx

src/components/markdown_editor/markdown_editor_footer.tsx

  • line 178 message ultimately comes from props and is safe

src/components/markdown_editor/markdown_format.tsx

  • line 60 result needs to be marked as safe

src/components/search_bar/filters/field_value_selection_filter.tsx

src/components/search_bar/filters/field_value_toggle_filter.tsx

  • line 70 this.resolveDisplay needs to be marked safe

src/components/search_bar/filters/field_value_toggle_group_filter.tsx

  • line 88 this.resolveDisplay needs to be marked safe

src/components/search_bar/filters/is_filter.tsx

  • line 65 this.resolveDisplay needs to be marked safe

src/components/selectable/selectable.tsx

  • line 605 messageContent is a ReactNode but being flagged as a string

src/components/selectable/selectable_list/selectable_list.tsx

src/components/steps/step_number.tsx

  • line 93 screenReaderText is safe, perhaps we update the lint rule to understand the useI18n*Step hooks as okay?

src/components/tabs/tab.tsx

  • line 105 prependNode is a ReactElement but flagged as a string
  • line 107 appendNode is a ReactElement but flagged as a string

Miscellaneous

src/components/date_picker/date_picker_range.tsx

  • line 117 uses an arrow character, . I dunno if arrows should be localized or not, need to research.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5399/

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5399/

@github-actions github-actions bot removed the stale-pr label Apr 6, 2022
@github-actions
Copy link

github-actions bot commented Jul 5, 2022

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@chandlerprall
Copy link
Contributor Author

Nothing like a stale check to remind yourself of the continual passage of time

@github-actions
Copy link

github-actions bot commented Oct 4, 2022

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@github-actions
Copy link

github-actions bot commented Jan 3, 2023

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

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.

3 participants