-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes index mapping api response to plural form #66012
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
Nice work @yuliacech! Tested locally. Changes LGTM.
I like your suggestion here: #59085 (comment). I think it makes sense to update the tab as well. Do you mind fixing that as part of this PR?
Hi @alisonelizabeth, thank you so much for your comment! |
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.
Thanks @yuliacech for making the change! LGTM.
I think it’s OK to leave the i18n ID the same. If we change it, I believe it would require the translations team to translate it again. AFAIK we don’t typically manually edit the translation files. You can also run this by the kibana localization team to confirm 😄.
For future though, if you do need to remove an i18n string, you can run:
node scripts/i18n_check --fix
to programmatically remove it. You can find more info on the i18n readme and guidelines pages.
@yuliacech Just noticed CI failed after I approved. It looks like you will also need to update the jest snapshot for the The other test failure looks unrelated and you may just need to update your branch. |
73797a5
to
dd8a4b6
Compare
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.
Changes LTGM! I haven't tested locally.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* changes index mapping api response to plural form * changes tab title to "Mappings" in Index Management detail panel * updates index management jest snapshots for "Mappings" title # Conflicts: # x-pack/test/api_integration/apis/management/index_management/mapping.js
* changes index mapping api response to plural form * changes tab title to "Mappings" in Index Management detail panel * updates index management jest snapshots for "Mappings" title # Conflicts: # x-pack/test/api_integration/apis/management/index_management/mapping.js
* changes index mapping api response to plural form * changes tab title to "Mappings" in Index Management detail panel * updates index management jest snapshots for "Mappings" title # Conflicts: # x-pack/test/api_integration/apis/management/index_management/mapping.js
* changes index mapping api response to plural form * changes tab title to "Mappings" in Index Management detail panel * updates index management jest snapshots for "Mappings" title # Conflicts: # x-pack/test/api_integration/apis/management/index_management/mapping.js
* changes index mapping api response to plural form * changes tab title to "Mappings" in Index Management detail panel * updates index management jest snapshots for "Mappings" title # Conflicts: # x-pack/test/api_integration/apis/management/index_management/mapping.js
* changes index mapping api response to plural form * changes tab title to "Mappings" in Index Management detail panel * updates index management jest snapshots for "Mappings" title # Conflicts: # x-pack/test/api_integration/apis/management/index_management/mapping.js
This PR changes the API response from
{mapping: ...}
to{mappings: ...}
in Index Management server code.Fixes #59085.
Release Note
We fixed
mappings
keyword in Index Management plugin (Index detail pane, Mapping tab).