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

[Rollups] Migrate to new page layout #102268

Merged

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Jun 15, 2021

This PR migrates Rollup Jobs to use the new page layout. I also made some minor UX changes - added a doc link to the list view, moved the action button to the right of the search bar, and removed the error toast notification (similar to my comment on #102042 (review)).

Related to #100748

Screenshots

List view
Screen Shot 2021-06-15 at 3 29 25 PM
Screen Shot 2021-06-15 at 3 33 02 PM
Screen Shot 2021-06-15 at 3 33 41 PM
Screen Shot 2021-06-16 at 9 55 55 AM
Screen Shot 2021-06-16 at 9 56 19 AM

Creation wizard
Screen Shot 2021-06-16 at 10 00 41 AM
Screen Shot 2021-06-16 at 10 01 40 AM

@alisonelizabeth alisonelizabeth added chore v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Rollups v7.14.0 labels Jun 15, 2021
/**
* 1. Ensure panel fills width of parent when search input yields no matching rollup jobs.
*/
.rollupJobsListPanel {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is necessary anymore; I included a screenshot in the PR description when there are no matching results.

@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alisonelizabeth alisonelizabeth marked this pull request as ready for review June 17, 2021 14:55
@alisonelizabeth alisonelizabeth requested review from a team as code owners June 17, 2021 14:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

@alisonelizabeth alisonelizabeth added the release_note:skip Skip the PR/issue when compiling release notes label Jun 17, 2021
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

docLinks updates LGTM

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Nice one @alisonelizabeth! Looks all good to me. Left two small comments, I do feel we should try to use the same loading component we use for the other apps. Besides that 🚀


{saveErrorFeedback}
<EuiPageContentBody restrictWidth style={{ width: '100%' }}>
<EuiPageHeader
Copy link
Member

Choose a reason for hiding this comment

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

Looks good! But wondering if we should ask EUI whether if this should have a borderBottom prop plus a large spacer under the header as in the other pages or if it's alright as is now? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Yes, I believe it should - fixed.

</EuiFlexItem>
</EuiFlexGroup>
<EuiPageContent verticalPosition="center" horizontalPosition="center" color="subdued">
<EuiEmptyPrompt
Copy link
Member

Choose a reason for hiding this comment

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

Could we use the SectionLoading from `/src/plugins/es_ui_shared/public/' instead? that way we will keep the loading states consistent throughout our apps

Copy link
Contributor

@dborodyansky dborodyansky left a comment

Choose a reason for hiding this comment

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

@alisonelizabeth Looks great! Only thing I notice is some inconsistent horizontal space in the creation screens. Is this expected?
image

@alisonelizabeth
Copy link
Contributor Author

@alisonelizabeth Looks great! Only thing I notice is some inconsistent horizontal space in the creation screens. Is this expected?

Thanks for the review @dborodyansky! Can you be more specific around the horizontal spacing? There's currently an <EuiSpacer size="l" /> between the page header and steps component, and steps component and form.

Screen Shot 2021-06-22 at 10 51 01 AM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
rollup 230.2KB 229.9KB -268.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 411.4KB 411.5KB +43.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@dborodyansky dborodyansky left a comment

Choose a reason for hiding this comment

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

@alisonelizabeth My mistake. I overlooked restrictWidth-default. Everything looks 👍 to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Rollups release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants