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

[EuiPagination] Add keys for i18n values in compressed count #8243

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

Conversation

acstll
Copy link

@acstll acstll commented Dec 16, 2024

Summary

This fixes #8235

I was able to test locally in Kibana, and this fix makes the console warning go away.

⚠️ I was not able to reproduce the error outside of Kibana, nor determine the real cause of it, see elastic/kibana#202287 — I do not fully yet understand how EuiI18n works.

I believe usage of EuiPortal, as suggested here, is not related to the issue.

QA

To trigger the warning, go to Discover and open a document (flyout).

Also maybe worth noting, the warning is only triggered the first time the flyout is open.

In order to test locally, you need to run EUI locally in Kibana, as explained here, taking into account the new workspace interdependency which is not yet documented. Follow these steps (thanks again @mgadewoll):

  • in packages/eui change eui-theme-common in the package.json from dependency to devDependency and add it also as peerDependency
  • run yarn build:workspaces in packages/eui
  • create local packages of all 3 packages: eui-theme-common, eui-theme-borealis and eui by running yarn build-pack in each directory
  • copy the created tgz packages to kibana root
  • add the packages in the Kibana package.json, e.g. like this (the names of the packages don't matter, just make sure to update the name if you create new packages so that the Kibana bundler registers a change and doesn't load a cached package)

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

@acstll acstll marked this pull request as ready for review December 16, 2024 15:26
@acstll acstll requested a review from a team as a code owner December 16, 2024 15:26
@acstll acstll changed the base branch from eui-theme/borealis to main December 17, 2024 09:14
@acstll acstll force-pushed the pagination-i18n-keys branch from 13b99fd to 8a4475e Compare December 17, 2024 09:14
@acstll acstll marked this pull request as draft December 17, 2024 09:26
@acstll
Copy link
Author

acstll commented Dec 17, 2024

Marking this as draft to do some more testing and make sure the fix is solid.

@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed a bug in Kibana where the `values` of an `EuiI18n` component inside `EuiPagination` would cause a ["unique key" warning](https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key). See https://github.com/elastic/kibana/issues/202287#issuecomment-2530968121
Copy link
Contributor

@mgadewoll mgadewoll Dec 17, 2024

Choose a reason for hiding this comment

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

I'd suggest not making this Kibana specific (While we noticed it in Kibana, it might as well affect other consumers). Let's phrase it a bit more generically to the actual change, maybe more like:

- Ensures that the `values` of `EuiI18n` used in `EuiPagination` use `key` attributes to prevent potential "unique key" warnings

Copy link
Author

Choose a reason for hiding this comment

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

amazing, thank you! I updated the message…

@acstll
Copy link
Author

acstll commented Dec 17, 2024

Quick status update: still making one last effort to reproduce the bug outside of Kibana 😅

@acstll acstll force-pushed the pagination-i18n-keys branch from 3b787ce to cd1f9e9 Compare December 17, 2024 18:07
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@acstll
Copy link
Author

acstll commented Dec 18, 2024

On the one hand, I still couldn't reproduce this outside of Kibana. I tried:

On the other hand, I can confirm the fix is not causing any other issues even with multiple instances of EuiPagination on the same page.

I would suggest we go ahead with this, in order to remove the warning in Kibana; and create a ticket to further explore the bug later on. I'm guessing it's coming from the logic in EuiI18n

@mgadewoll what do you think?

@acstll acstll marked this pull request as ready for review December 18, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiPagination] Each child in a list should have a unique "key" prop
4 participants