-
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
[Reporting] Reporting info panel touch ups #120617
[Reporting] Reporting info panel touch ups #120617
Conversation
…ng. still need to re-introduce errors and deprecated report types in the new structure
…d settings format
@elasticmachine merge upstream |
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
Pinging @elastic/kibana-app-services (Team:AppServicesUx) |
@elasticmachine merge upstream |
x-pack/plugins/reporting/public/management/components/report_info_flyout_content.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/reporting/public/management/components/report_info_flyout_content.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/reporting/public/management/components/report_info_flyout_content.tsx
Outdated
Show resolved
Hide resolved
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 left a few minor suggestions
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
@elasticmachine merge upstream |
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
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.
Is it possible to add more spacing between the title "Timestamps" and "Time zone"? It seems a little tight.
}} | ||
> | ||
{i18n.translate('xpack.reporting.reportInfoFlyout.openInKibanaAppButtonLabel', { | ||
defaultMessage: 'Open in Kibana App', |
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 don't really have the context for this text, but can it be: Open in Kibana
If you do include app, it should be lower case.
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.
"Open in Kibana" sounds good!
|
||
hasScreenshot && { | ||
title: i18n.translate('xpack.reporting.listing.infoPanel.dimensionsInfoHeight', { | ||
defaultMessage: 'Height (in pixels)', |
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.
Are the parens needed? "Size in bytes" does not contain parens.
}, | ||
hasScreenshot && { | ||
title: i18n.translate('xpack.reporting.listing.infoPanel.dimensionsInfoWidth', { | ||
defaultMessage: 'Width (in pixels)', |
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.
Same comment about the parens. Does this work:
Width in pixels
@@ -431,10 +429,7 @@ class ReportListingUi extends Component<Props, State> { | |||
defaultMessage: 'Open the Kibana App where this report was generated.', |
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.
defaultMessage: 'Open the Kibana App where this report was generated.', | |
defaultMessage: 'Open the Kibana app where this report was generated.', |
Thanks for the review @gchaps ! I think I've addressed your points.
Great idea, updated, see the screenshot. |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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
* changed info -> summary and combined maximum attempts and attempts field * added spaceId if it is there to job info * moved space id to job details section * moved processing and job info into a single array * wip, introduced a new structure to the flyout for easier visual parsing. still need to re-introduce errors and deprecated report types in the new structure * remove unnecessary div * added warnings and errors to callout and cleaned up comments * remove unused functionality and introduce date formatting per advanced settings format * remove unused import * remove unused i18n * refactor check for info.max_attempts in flyout content Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * update info.lyout.dimensions.height to be Math.ceiled Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * update info.lyout.dimensions.width to be Math.ceiled Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * lint and remove bad suggestion commit * added actions button to flyout, in flyout footer * create a little bit more breathing room for the section titles * added logic to disable the action buttons in the flyout when the actions are not ready * update copy per feedback * added basic tests for the actions menu in the flyout Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
* changed info -> summary and combined maximum attempts and attempts field * added spaceId if it is there to job info * moved space id to job details section * moved processing and job info into a single array * wip, introduced a new structure to the flyout for easier visual parsing. still need to re-introduce errors and deprecated report types in the new structure * remove unnecessary div * added warnings and errors to callout and cleaned up comments * remove unused functionality and introduce date formatting per advanced settings format * remove unused import * remove unused i18n * refactor check for info.max_attempts in flyout content Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * update info.lyout.dimensions.height to be Math.ceiled Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * update info.lyout.dimensions.width to be Math.ceiled Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * lint and remove bad suggestion commit * added actions button to flyout, in flyout footer * create a little bit more breathing room for the section titles * added logic to disable the action buttons in the flyout when the actions are not ready * update copy per feedback * added basic tests for the actions menu in the flyout Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> (cherry picked from commit d213263) # Conflicts: # x-pack/plugins/reporting/public/lib/job.tsx # x-pack/plugins/reporting/public/management/components/report_info_flyout_content.tsx # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json
* [Reporting] Reporting info panel touch ups (#120617) * changed info -> summary and combined maximum attempts and attempts field * added spaceId if it is there to job info * moved space id to job details section * moved processing and job info into a single array * wip, introduced a new structure to the flyout for easier visual parsing. still need to re-introduce errors and deprecated report types in the new structure * remove unnecessary div * added warnings and errors to callout and cleaned up comments * remove unused functionality and introduce date formatting per advanced settings format * remove unused import * remove unused i18n * refactor check for info.max_attempts in flyout content Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * update info.lyout.dimensions.height to be Math.ceiled Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * update info.lyout.dimensions.width to be Math.ceiled Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> * lint and remove bad suggestion commit * added actions button to flyout, in flyout footer * create a little bit more breathing room for the section titles * added logic to disable the action buttons in the flyout when the actions are not ready * update copy per feedback * added basic tests for the actions menu in the flyout Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> (cherry picked from commit d213263) # Conflicts: # x-pack/plugins/reporting/public/lib/job.tsx # x-pack/plugins/reporting/public/management/components/report_info_flyout_content.tsx # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json * resolved merge conflicts
Summary
Closes #118247
Addresses the points raised the issue and introduces new structure to the flyout. Notable changes:
How to test
Release note
We introduced a new structure to the report details flyout to help readers get to the information they want faster.
Screenshots
Screenshots
With warnings
With errors
With errors and warnings