-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Rollup] Fix rendering undefined
from job status map
#41438
[Rollup] Fix rendering undefined
from job status map
#41438
Conversation
Added data to test case to ensure that the components using JobStatus can still render with unknown states
Pinging @elastic/es-ui |
💚 Build Succeeded |
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.
Great, thanks for adding this @jloleysens ! Could we add a test in the job_table.test.js
to verify that both the "stopping" and the "unknown" status are rendered correctly? The same way we currently do it on L90.
Cheers!
search_failures: 0, | ||
}, | ||
}, | ||
// Components using JobStatus should still be able to render |
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.
Can we move this comment above L72 and update it to make it clearer that "JobStatus" is a component?
// Components using the <JobStatus /> component should still be able to render
// despite having a rollup job with an unknown status
…b-panel-for-stopping-jobs * 'master' of github.com:elastic/kibana: (58 commits) [DOCS] Timelion cleanup (elastic#41381) [Docs] Add simple phrase highlighting to Logs UI (elastic#41610) [Maps] Rename modules for clarity (elastic#41608) [Monitoring] Metricbeat migration net new user experience (elastic#39832) [Maps] Only color legend icon with dynamic color when dynamic config is complete (elastic#41607) [TSVB] [Markdown] markdown section do not render after change data parameter (elastic#41576) [Vega] (Step 2) Shim new platform - renaming vega -> vis_type_vega (elastic#41565) update dark mode tsvb test (elastic#41618) [i18n] .i18nrc file as the source of truth and enhance tooling (elastic#39774) Reactify Top Nav Menu (kbn_top_nav) (elastic#40262) fix(code/frontend): should update search results if search options change (elastic#41232) Use kibana-ci-proxy-cache for chrome and gecko drivers (elastic#41581) [SIEM] Fix draggables to work with escapeId for the ML severity column (elastic#41621) [Canvas] Updates esdocs default count to 1000 (elastic#41604) [Uptime] Fix duration chart for Safari (elastic#41619) [Canvas] Restores "Today" as a quick time range in time filter (elastic#41528) docs: lowercase app (elastic#41612) [Code] Update git repository update frequency (elastic#41541) Remove language=json on code blocks due to performance hit (elastic#41540) [DOCS] Update anchors and links for Elasticserach API relocation. (elastic#41372) ...
💚 Build Succeeded |
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.
Great work @jloleysens! I tested this locally and it worked as expected, and the code LGTM! I had one comment on the translations.
Also, I added a section called "Release note" to the PR description. Every PR which we label with a "release-note" label should have a section like this so that whoever is putting together our release notes can just aggregate this content from each PR.
Also, in the future would you mind adding screenshots to PRs which merit them? This helps reviewers understand the change at a glance and know what to expect when testing locally. Sometimes these screenshots are also used in release notes and blogs.
@@ -8977,6 +8977,7 @@ | |||
"xpack.rollupJobs.jobStatus.indexingLabel": "索引", | |||
"xpack.rollupJobs.jobStatus.startedLabel": "已开始", | |||
"xpack.rollupJobs.jobStatus.stoppedLabel": "已停止", | |||
"xpack.rollupJobs.jobStatus.unknownLabel": "未知", |
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.
We hire professional translators to add translations for us. Did you add these translations yourself? If so, I think you can remove them.
@cjcenizal careful using words like "caca" in our screenshots that might endup in a blog post. French (and other) speaking languages users might not like it 😊 |
💚 Build Succeeded |
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.
LGTM! Thanks for the changes
* Updated JobStatus' status mapper to handle unknown strings Added data to test case to ensure that the components using JobStatus can still render with unknown states * Fixed unnecessary formatting changes and added translation for unknown label JP * Added Chinese translation for unknown * Added unknown status spec * Remove manual addition of translation
* Updated JobStatus' status mapper to handle unknown strings Added data to test case to ensure that the components using JobStatus can still render with unknown states * Fixed unnecessary formatting changes and added translation for unknown label JP * Added Chinese translation for unknown * Added unknown status spec * Remove manual addition of translation
Summary
Fixes #36795.
Added a new
JobStatus
for the (unlikely?) case that we get unknown job statuses in future.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Documentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers
Release note
Previously, if any rollup job had the "stopping" state, it would cause the Rollup Jobs table to mis-render as an empty page. Now, the table renders correctly and this state is surfaced correctly in the table.