-
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
[Rollups] Migrate to new page layout #102268
[Rollups] Migrate to new page layout #102268
Conversation
/** | ||
* 1. Ensure panel fills width of parent when search input yields no matching rollup jobs. | ||
*/ | ||
.rollupJobsListPanel { |
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 think this is necessary anymore; I included a screenshot in the PR description when there are no matching results.
⏳ Build in-progress, with failures
Failed CI StepsHistory
To update your PR or re-run it, just comment with: |
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
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.
docLinks updates 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.
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 |
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.
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? 🤔
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.
Good catch! Yes, I believe it should - fixed.
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
<EuiPageContent verticalPosition="center" horizontalPosition="center" color="subdued"> | ||
<EuiEmptyPrompt |
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.
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
…ps/new_page_layout
…ps/new_page_layout
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.
@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 |
…ps/new_page_layout
💚 Build SucceededMetrics [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.
@alisonelizabeth My mistake. I overlooked restrictWidth-default
. Everything looks 👍 to me.
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
Creation wizard