-
Notifications
You must be signed in to change notification settings - Fork 841
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
base: main
Are you sure you want to change the base?
Conversation
e162ad1
to
8e7e81c
Compare
13b99fd
to
8a4475e
Compare
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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…
Quick status update: still making one last effort to reproduce the bug outside of Kibana 😅 |
I was not able to reproduce the error outside Kibana, nor determine the real cause of it, see elastic/kibana#202287
3b787ce
to
cd1f9e9
Compare
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
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 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 @mgadewoll what do you think? |
Summary
This fixes #8235
I was able to test locally in Kibana, and this fix makes the console warning go away.
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):
packages/eui
changeeui-theme-common
in the package.json fromdependency
todevDependency
and add it also aspeerDependency
yarn build:workspaces
in packages/euieui-theme-common
,eui-theme-borealis
andeui
by runningyarn build-pack
in each directoryGeneral checklist
Checked in both light and dark modesChecked in mobileChecked in Chrome, Safari, Edge, and FirefoxChecked for accessibility including keyboard-only and screenreader modesAdded documentationProps have proper autodocs (using@default
if default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesAdded or updated jest and cypress testsUpdated visual regression testsIf applicable, added the breaking change issue label (and filled out the breaking change 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)