[Snapshot + Restore] Migrate to new page layout structure by alisonelizabeth · Pull Request #101811 · elastic/kibana

alisonelizabeth · 2021-06-09T18:36:53Z

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

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.

t

Privileges check (all routes)

Create policy

Edit policy

Add repository

Edit repository

Restore snapshot

alisonelizabeth · 2021-06-10T15:45:49Z

@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

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

alisonelizabeth · 2021-06-10T15:56:02Z

x-pack/plugins/snapshot_restore/public/application/sections/policy_edit/policy_edit.tsx

  const renderLoading = () => {
-    return errorLoadingPolicy ? (
-      <SectionLoading>
+    return isLoadingPolicy ? (


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 · 2021-06-14T12:38:58Z

@elasticmachine merge upstream

elasticmachine · 2021-06-14T14:51:03Z

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

cjcenizal · 2021-06-14T17:37:14Z

@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?

alisonelizabeth · 2021-06-14T18:20:34Z

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

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

cjcenizal · 2021-06-15T03:50:52Z

src/plugins/es_ui_shared/__packages_do_not_import__/authorization/components/page_error.tsx

+  title,
+  error,
+  actions,
+  centerErrorContent,


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.

👍 agreed - fixed.

cjcenizal · 2021-06-15T03:53:04Z

x-pack/plugins/snapshot_restore/public/application/components/index.ts

 export { RepositoryVerificationBadge } from './repository_verification_badge';
 export { RepositoryTypeLogo } from './repository_type_logo';
-export { SectionLoading } from './section_loading';
+export { SectionLoading, InlineLoading, PageLoading } from './loading';


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 · 2021-06-15T13:17:17Z

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 · 2021-06-15T13:35:47Z

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 · 2021-06-15T15:39:10Z

💚 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

💚 Build #130901 succeeded eeffee5
💚 Build #130356 succeeded 3af2d92
💔 Build #130263 failed 7679378

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

…102232) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…1811)

migrate SR to new page layout structure

7679378

alisonelizabeth mentioned this pull request Jun 9, 2021

Convert Stack Management apps to new Management nav structure #100748

Closed

10 tasks

fix import

3af2d92

alisonelizabeth commented Jun 10, 2021

View reviewed changes

Merge branch 'master' into sr/new_page_layout

eeffee5

alisonelizabeth marked this pull request as ready for review June 14, 2021 14:51

alisonelizabeth requested a review from a team as a code owner June 14, 2021 14:51

alisonelizabeth requested a review from cjcenizal June 14, 2021 14:51

cjcenizal approved these changes Jun 15, 2021

View reviewed changes

address review feedback

ff77a9b

alisonelizabeth merged commit 6173c04 into elastic:master Jun 15, 2021

alisonelizabeth deleted the sr/new_page_layout branch June 15, 2021 15:39

alisonelizabeth mentioned this pull request Jun 15, 2021

[7.x] [Snapshot + Restore] Migrate to new page layout structure (#101811) #102232

Merged

alisonelizabeth added a commit that referenced this pull request Jun 15, 2021

[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

[Snapshot + Restore] Migrate to new page layout structure (elastic#10…

95541e6

…1811)

Conversation

alisonelizabeth commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Uh oh!

alisonelizabeth commented Jun 10, 2021

Uh oh!

alisonelizabeth Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth commented Jun 14, 2021

Uh oh!

elasticmachine commented Jun 14, 2021

Uh oh!

cjcenizal commented Jun 14, 2021

Uh oh!

alisonelizabeth commented Jun 14, 2021

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos commented Jun 15, 2021

Uh oh!

alisonelizabeth commented Jun 15, 2021

Uh oh!

kibanamachine commented Jun 15, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alisonelizabeth commented Jun 9, 2021 •

edited

Loading