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

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

acstll
Copy link
Contributor

@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
Contributor Author

acstll commented Dec 17, 2024

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

@acstll
Copy link
Contributor 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
@acstll
Copy link
Contributor 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
@mgadewoll
Copy link
Contributor

@mgadewoll what do you think?

Yes, I'm fine with this. Adding keys should not be an issue. We can dig deeper if needed in the future.
Just to be sure: Did you confirm that multiple instances don't cause trouble in EUI and Kibana?

@acstll
Copy link
Contributor Author

acstll commented Jan 7, 2025

Just to be sure: Did you confirm that multiple instances don't cause trouble in EUI and Kibana?

I did in EUI but I'm not sure now if I actually did in Kibana… Will double-check to be extra sure 👍

@acstll acstll force-pushed the pagination-i18n-keys branch from cd1f9e9 to d3e7e5e Compare January 7, 2025 11:39
@acstll
Copy link
Contributor Author

acstll commented Jan 7, 2025

@mgadewoll no trouble 👍

Screenshot 2025-01-07 at 12 28 59

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Thanks for digging into this! 🎉

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@acstll acstll merged commit 41cf816 into elastic:main Jan 7, 2025
5 checks passed
@acstll acstll deleted the pagination-i18n-keys branch January 7, 2025 13:50
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