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.

Sign up for GitHub

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

Jump to bottom

[Snapshot + Restore] Migrate to new page layout structure #101811

Merged
merged 4 commits into from
Jun 15, 2021
Merged

[Snapshot + Restore] Migrate to new page layout structure #101811

merged 4 commits into from
Jun 15, 2021

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Jun 9, 2021 •
edited
Loading

This PR migrates Snapshot + Restore to use the new page layout.

Related to #100748

As part of this work, I added a new PageError component to es_ui_shared to handle full-page errors.

Screenshots

Screenshots

Policy list
Note: I did not take screenshots of every tab, as the layout change was made in home.tsx and the behavior should be the same for all tabs.

Screen Shot 2021-06-10 at 10 32 01 AM t Screen Shot 2021-06-10 at 10 28 56 AM Screen Shot 2021-06-10 at 10 29 22 AM Screen Shot 2021-06-10 at 10 28 28 AM

Privileges check (all routes)
Screen Shot 2021-06-10 at 10 34 00 AM
Screen Shot 2021-06-10 at 10 34 53 AM

Create policy
Screen Shot 2021-06-10 at 10 37 42 AM
Screen Shot 2021-06-10 at 10 41 03 AM
Screen Shot 2021-06-10 at 10 41 28 AM

Edit policy
Screen Shot 2021-06-10 at 11 32 17 AM
Screen Shot 2021-06-10 at 10 44 07 AM
Screen Shot 2021-06-10 at 10 44 52 AM

Add repository
Screen Shot 2021-06-10 at 10 48 08 AM

Edit repository
Screen Shot 2021-06-10 at 10 48 57 AM
Screen Shot 2021-06-10 at 10 49 44 AM
Screen Shot 2021-06-10 at 10 50 12 AM

Restore snapshot
Screen Shot 2021-06-10 at 10 59 49 AM
Screen Shot 2021-06-10 at 11 00 29 AM
Screen Shot 2021-06-10 at 11 07 58 AM

@alisonelizabeth alisonelizabeth added chore v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI v7.14.0 labels Jun 9, 2021
@alisonelizabeth alisonelizabeth mentioned this pull request Jun 9, 2021
Closed
10 tasks
@alisonelizabeth
Copy link
Contributor Author

@cchaos / @cjcenizal curious on your thoughts on how to best format content within a step-based wizard.

I started applying the restrictWidth prop, as in #101548. However, I ended up with different widths for different steps, which felt a little jarring.

With restrictWidth
Screen Shot 2021-06-10 at 10 37 42 AM
Screen Shot 2021-06-10 at 10 37 33 AM

Without it, I feel like the "Cancel" button sometimes get a little lost at the bottom. But maybe I just need to get used to the new layout 😄 .

Without restrictWidth
Screen Shot 2021-06-10 at 11 41 16 AM
Screen Shot 2021-06-10 at 11 41 24 AM

alisonelizabeth
alisonelizabeth commented Jun 10, 2021
View reviewed changes
x-pack/plugins/snapshot_restore/public/application/sections/policy_edit/policy_edit.tsx
const renderLoading = () => {
return errorLoadingPolicy ? (
<SectionLoading>
return isLoadingPolicy ? (
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 was behaving correctly before. I don't see why we would want to check errorLoadingPolicy to determine whether we should render a loading state or not.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

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

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

@cjcenizal
Copy link
Contributor

@alisonelizabeth I remember noticing that weird behavior with restrictWidth, too. @cchaos Is this something that can be fixed on the EUI side?

Alison, I noticed the breadcrumbs aren't updated in the privileges checks, making the UX seem a little disconnected from the rest of Kibana. If this is existing behavior, mind creating an issue?

Screen Shot 2021-06-10 at 10 34 00 AM

Screen Shot 2021-06-10 at 10 34 53 AM

@alisonelizabeth
Copy link
Contributor Author

Alison, I noticed the breadcrumbs aren't updated in the privileges checks, making the UX seem a little disconnected from the rest of Kibana. If this is existing behavior, mind creating an issue?

Great catch @cjcenizal! This does appear to be an existing bug. I opened #102109.

cjcenizal
cjcenizal approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Thanks for getting this one migrated! Tested locally, code LGTM.

src/plugins/es_ui_shared/__packages_do_not_import__/authorization/components/page_error.tsx Outdated
title,
error,
actions,
centerErrorContent,
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT of using a convention of prefixing boolean flags with is, can, and so on, to make their role clearer? For example, isCentered reads more clearly to me as a flag, vs. centerErrorContent which sounds more like a function due to the initial verb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 agreed - fixed.

x-pack/plugins/snapshot_restore/public/application/components/index.ts
@@ -11,7 +11,7 @@ export { RepositoryDeleteProvider } from './repository_delete_provider';
export { RepositoryForm } from './repository_form';
export { RepositoryVerificationBadge } from './repository_verification_badge';
export { RepositoryTypeLogo } from './repository_type_logo';
export { SectionLoading } from './section_loading';
export { SectionLoading, InlineLoading, PageLoading } from './loading';
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the addition of these components. In the future we can explore moving them to es_ui_shared and using them broadly across our apps.

@cchaos
Copy link
Contributor

cchaos commented Jun 15, 2021

Looks like when a restrictWidth component is a descendent of a flex item, it will shrink to fit the contents. You can add width: 100% as a style attribute to ensure it grows. Otherwise I can take a look in EUI, but it’s a long upgrade path

@alisonelizabeth
Copy link
Contributor Author

Looks like when a restrictWidth component is a descendent of a flex item, it will shrink to fit the contents. You can add width: 100% as a style attribute to ensure it grows. Otherwise I can take a look in EUI, but it’s a long upgrade path

Thanks @cchaos! I've added width:100% for now, however, if it is something that can be addressed via EUI at some point I think that would be preferred.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
esUiShared 152 153 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
esUiShared 86 88 +2

Async chunks

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

id before after diff
snapshotRestore 459.5KB 458.9KB -595.0B

Page load bundle

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

id before after diff
esUiShared 189.8KB 191.8KB +2.0KB
snapshotRestore 40.6KB 40.9KB +370.0B
total +2.3KB
Unknown metric groups

API count

id before after diff
esUiShared 88 90 +2

History

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

@alisonelizabeth alisonelizabeth merged commit 6173c04 into elastic:master Jun 15, 2021
@alisonelizabeth alisonelizabeth deleted the sr/new_page_layout branch June 15, 2021 15:39
@alisonelizabeth alisonelizabeth mentioned this pull request Jun 15, 2021
Merged
alisonelizabeth added a commit that referenced this pull request Jun 15, 2021
@alisonelizabeth @kibanamachine
[Snapshot + Restore] Migrate to new page layout structure (#101811) (#…
2841e38 
…102232)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
cuff-links pushed a commit to cuff-links/kibana that referenced this pull request Jun 15, 2021
@alisonelizabeth
[Snapshot + Restore] Migrate to new page layout structure (elastic#10…
95541e6 
…1811)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcenizal cjcenizal cjcenizal approved these changes

Assignees
No one assigned
Labels
chore Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alisonelizabeth @elasticmachine @cjcenizal @cchaos @kibanamachine