Reducing wrapping divs from RenderingService #97017

cchaos · 2021-04-13T17:40:37Z

Page Layout Service Part 1: Reduce/Reuse/Recycle

Reducing the number of wrapping elements before getting to actual page content.

It seems that over the years, instead of fixing unnecessary layers, more continued to get added creating lots of nested divs. This is trouble for the types of layouts we try to create using flex to stretch the contents. The following is the old vs new rendered DOM:

Essentially it boils down to:

1. Reducing the wrappers in the rendering service

Removed the .content wrapper
- There were no styles attached to this element
- There was a data-test-subj="kibanaChrome" which is targeted by some tests, but I've moved this to the #kibana-body element (one level above). Iffy on whether that was really the appropriate element to move it to or if it needs to be within the service.
Removed the extraneous .app-wrapper-panel wrapping element
Removed the <AppContainer /> wrapper which applied a way-too-generic className of .application and is how plugins passed down the applicationClasses.
- I've moved these classes to be applied to the <AppWrapper /> instead, but removed .application in favor of .kbnAppWrapper that most plugins can now just grab from the global variable APP_WRAPPER_CLASS.

The result of which is

kibana/src/core/public/rendering/rendering_service.tsx

Lines 49 to 72 in 77a7f72

    
           ReactDOM.render( 
        
             <I18nProvider> 
        
               <> 
        
                 {/* Fixed headers */} 
        
                 {chromeHeader} 
        
                 {/* banners$.subscribe() for things like the No data banner */} 
        
                 <div id="globalBannerList">{bannerComponent}</div> 
        
                 {/* The App Wrapper outside of the fixed headers that accepts custom class names from apps */} 
        
                 <AppWrapper 
        
                   chromeVisible$={chrome.getIsVisible$()} 
        
                   classes$={chrome.getApplicationClasses$()} 
        
                 > 
        
                   {/* Affixes a div to restrict the position of charts tooltip to the visible viewport minus the header */} 
        
                   <div id="app-fixed-viewport" /> 
        
                   {/* The actual plugin/app */} 
        
                   {appComponent} 
        
                 </AppWrapper> 
        
               </> 
        
             </I18nProvider>, 
        
             targetDomElement 
        
           );

2. Added a better and reusable class name for any elements wrapping applications

The AppWrapper component can now be reused by individual applications to ensure proper layout, or they can use the APP_WRAPPER_CLASS exported variable.

The variable is for places like Dashboard still creating elements through the document.

kibana/src/plugins/dashboard/public/plugin.tsx

Line 294 in 77a7f72

params.element.classList.add(APP_WRAPPER_CLASS);

3. Updated some naming of classes just to be more consistant:

.app-wrapper -> .kbnAppWrapper

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Some being temporary and will need a better solution when introducing the page layout component

Can’t figure out how to have a optional Observable `Received: "kbnAppWrapper class-name”`

…_1/reduce_wrappers # Conflicts: # src/core/public/rendering/_base.scss # src/core/public/rendering/rendering_service.tsx

cchaos

@pgayvallet Thank you for your help on this one. I've added the places where it's failing and I don't know how to fix it below.

src/core/public/rendering/app_containers.tsx

src/core/public/rendering/rendering_service.test.tsx

x-pack/plugins/reporting/server/lib/screenshots/constants.ts

cchaos · 2021-04-14T16:40:53Z

Additional Fixes in this PR:

Global banners

Fixed, the left position when the global nav drawer is docked and add truncation so the text wouldn't extend beyond the established height of the banner.

…to use it

…_1/reduce_wrappers

…pers move APP_WRAPPER_CLASS definition to src/core/common

cchaos · 2021-04-15T18:27:27Z

Reviewers:

Please, please, please, check your application pages to ensure the layouts are still working correctly. Especially, if you were targeting or relying any of the following classes:

The following application wrappers needing/applying flex have been removed

.content
.application
.app-wrapper
.app-wrapper-panel, .app-wrapper-panel > *

Import and use APP_WRAPPER_CLASS from core/public or .kbnAppWrapper.

The class used for checking for hidden chrome has been renamed

.hidden-chrome -> .kbnAppWrapper--hiddenChrome

Going forward

If your application also has another custom class simply to provide column, flex layout for stretching you can replace your custom class with the APP_WRAPPER_CLASS from core/public.

wylieconlon

I tested the Kibana App owned apps in Firefox and Safari and wasn't able to find any issues. LGTM

elasticmachine · 2021-04-15T19:15:53Z

Pinging @elastic/uptime (Team:uptime)

pgayvallet

Tested locally, with banner disabled then enabled:

dashboard
dashboard full screen
visualize
maps
canvas
login page

All seems ok.

LGTM

src/core/public/core_system.ts

src/core/tsconfig.json

alvarezmelissa87 · 2021-04-19T20:13:32Z

Tested locally for ML - LGTM 👍

azasypkin

Changes in Platform Security functional tests LGTM.

sebelga

Tested the ES UI apps and didn't spot any regression 👍 Thanks for doing that @cchaos !

ThomThomson

~~Code only review: Presentation team changes LGTM!~~

Tested locally on chrome, Dashboard changes LGTM. Focused on dashboard layout, fullscreen mode, embed mode & overlays. Everything LGTM!

cchaos · 2021-04-26T15:33:31Z

@elasticmachine merge upstream

dominiqueclarke

Tested for Uptime and it LGTM, except we do have one view relying on flex from one of the removed wrappers.

We can create an issue to address this using the APP_WRAPPER_CLASS.
CC: @shahzad31 @justinkambic

kibanamachine · 2021-04-26T17:32:14Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	363	364	+1

Async chunks

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

id	before	after	diff
`canvas`	1.2MB	1.2MB	-288.0B
`dashboard`	138.9KB	138.8KB	-126.0B
`monitoring`	728.8KB	728.5KB	-286.0B
`securitySolution`	7.0MB	7.0MB	+2.0B
`uptime`	1.2MB	1.2MB	-31.0B
total			-729.0B

Page load bundle

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

id	before	after	diff
`banners`	11.7KB	11.7KB	+29.0B
`canvas`	518.4KB	518.4KB	+2.0B
`core`	404.1KB	400.9KB	-3.2KB
`dashboard`	313.0KB	313.0KB	+8.0B
total			-3.1KB

Unknown metric groups

API count

id	before	after	diff
`core`	2180	2182	+2

History

💛 Build #120713 was flaky 81341e4
💔 Build #120614 failed 5ae6413
💚 Build #120043 succeeded 1498900
💔 Build #119778 failed d75b4b8
💔 Build #119727 failed 6898e03

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

FrankHassanabad

LGTM

andreadelrio

Design changes LGTM 🎉

* Reducing wrapping divs from RenderingService * Applying more styles to .kbnAppWrapper Some being temporary and will need a better solution when introducing the page layout component * Almost fixing tests for rendering service Can’t figure out how to have a optional Observable `Received: "kbnAppWrapper class-name”` * Adding some comments * [Dashboard] Using the APP_WRAPPER_CLASS * fix test & ts types * Fixin a few more tests that were using `.app-wrapper` * Creating docs for new var and cleaning up some selectors * Fixing reporting * Fixing banner position and truncation * Fixed CSS error in loading screen and jump in animation * Fixing selectors in Canvas * Remove unused var * Added `APP_WRAPPER_CLASS` export from `server` and updated reporting to use it * Fix monitoring icon clicks * move APP_WRAPPER_CLASS definition to src/core/common * Fixing Monitoring snapshots and wrapper class * Moved `APP_WRAPPER_CLASS` utils but exported from `public` and `server` * Remove old folder * Fix dashboard test by only showing HR in edit mode Co-authored-by: pgayvallet Co-authored-by: tsullivan

cchaos added 6 commits April 5, 2021 14:19

Reducing wrapping divs from RenderingService

e0ee8e7

Applying more styles to .kbnAppWrapper

f177b78

Some being temporary and will need a better solution when introducing the page layout component

Almost fixing tests for rendering service

8d18bcf

Can’t figure out how to have a optional Observable `Received: "kbnAppWrapper class-name”`

Merge remote-tracking branch 'upstream/master' into pageLayouts/stage…

c7a3026

…_1/reduce_wrappers # Conflicts: # src/core/public/rendering/_base.scss # src/core/public/rendering/rendering_service.tsx

Adding some comments

2ad7f1a

[Dashboard] Using the APP_WRAPPER_CLASS

77a7f72

cchaos commented Apr 13, 2021

View reviewed changes

src/core/public/rendering/app_containers.tsx Show resolved Hide resolved

src/core/public/rendering/rendering_service.test.tsx Show resolved Hide resolved

pgayvallet and others added 8 commits April 14, 2021 09:29

fix test & ts types

a63a5fc

Fixin a few more tests that were using .app-wrapper

521ea8b

Creating docs for new var and cleaning up some selectors

58e8de4

Fixing reporting

03f8693

Fixing banner position and truncation

5b62969

Fixed CSS error in loading screen and jump in animation

e53a889

Fixing selectors in Canvas

8d84b4b

Remove unused var

83dae4d

cchaos commented Apr 14, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/screenshots/constants.ts Outdated Show resolved Hide resolved

cchaos and others added 7 commits April 14, 2021 14:16

Added APP_WRAPPER_CLASS export from server and updated reporting …

bbac6f7

…to use it

Fix monitoring icon clicks

9909be0

Merge remote-tracking branch 'upstream/master' into pageLayouts/stage…

6898e03

…_1/reduce_wrappers

move APP_WRAPPER_CLASS definition to src/core/common

1680999

Merge pull request #25 from tsullivan/pageLayouts/stage_1/reduce_wrap…

d75b4b8

…pers move APP_WRAPPER_CLASS definition to src/core/common

Fixing Monitoring snapshots and wrapper class

351e732

Moved APP_WRAPPER_CLASS utils but exported from public and server

1498900

cchaos added v7.13.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2021

cchaos marked this pull request as ready for review April 15, 2021 18:27

cchaos requested review from a team as code owners April 15, 2021 18:27

wylieconlon approved these changes Apr 15, 2021

View reviewed changes

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Apr 15, 2021

pgayvallet approved these changes Apr 19, 2021

View reviewed changes

src/core/public/core_system.ts Show resolved Hide resolved

src/core/tsconfig.json Outdated Show resolved Hide resolved

cchaos added 2 commits April 19, 2021 11:20

Remove old folder

dd0bf61

Merge branch 'master' into pageLayouts/stage_1/reduce_wrappers

5ae6413

cchaos added v7.14.0 and removed v7.13.0 labels Apr 19, 2021

Fix dashboard test by only showing HR in edit mode

81341e4

azasypkin approved these changes Apr 21, 2021

View reviewed changes

mdefazio mentioned this pull request Apr 22, 2021

Visualize: Listing page unbearably slow #92801

Closed

sebelga self-requested a review April 22, 2021 13:37

sebelga approved these changes Apr 22, 2021

View reviewed changes

ThomThomson approved these changes Apr 26, 2021

View reviewed changes

Merge branch 'master' into pageLayouts/stage_1/reduce_wrappers

2ccd399

dominiqueclarke approved these changes Apr 26, 2021

View reviewed changes

FrankHassanabad approved these changes Apr 26, 2021

View reviewed changes

andreadelrio approved these changes Apr 26, 2021

View reviewed changes

cchaos merged commit f07ebc8 into elastic:master Apr 26, 2021

cchaos mentioned this pull request Apr 26, 2021

[7.x] Reducing wrapping divs from RenderingService (#97017) #98377

Merged

This was referenced Apr 27, 2021

[Uptime] Check Details Page does not stretch to full view #98467

Closed

[Uptime] update uptime page styles #98936

Merged

cchaos mentioned this pull request Jun 9, 2021

Fix AppContainer layout #101793

Merged

3 tasks

cchaos deleted the pageLayouts/stage_1/reduce_wrappers branch March 11, 2022 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reducing wrapping divs from RenderingService #97017

Reducing wrapping divs from RenderingService #97017

cchaos commented Apr 13, 2021 •

edited

Loading

cchaos left a comment

cchaos commented Apr 14, 2021

cchaos commented Apr 15, 2021 •

edited

Loading

wylieconlon left a comment

elasticmachine commented Apr 15, 2021

pgayvallet left a comment

alvarezmelissa87 commented Apr 19, 2021

azasypkin left a comment

sebelga left a comment

ThomThomson left a comment •

edited

Loading

cchaos commented Apr 26, 2021

dominiqueclarke left a comment

kibanamachine commented Apr 26, 2021

API count

FrankHassanabad left a comment

andreadelrio left a comment

	ReactDOM.render(
	<I18nProvider>
	<>
	{/* Fixed headers */}
	{chromeHeader}

	{/* banners$.subscribe() for things like the No data banner */}
	<div id="globalBannerList">{bannerComponent}</div>

	{/* The App Wrapper outside of the fixed headers that accepts custom class names from apps */}
	<AppWrapper
	chromeVisible$={chrome.getIsVisible$()}
	classes$={chrome.getApplicationClasses$()}
	>
	{/* Affixes a div to restrict the position of charts tooltip to the visible viewport minus the header */}
	<div id="app-fixed-viewport" />

	{/* The actual plugin/app */}
	{appComponent}
	</AppWrapper>
	</>
	</I18nProvider>,
	targetDomElement
	);

Reducing wrapping divs from RenderingService #97017

Reducing wrapping divs from RenderingService #97017

Conversation

cchaos commented Apr 13, 2021 • edited Loading

Reducing the number of wrapping elements before getting to actual page content.

1. Reducing the wrappers in the rendering service

2. Added a better and reusable class name for any elements wrapping applications

3. Updated some naming of classes just to be more consistant:

Checklist

For maintainers

cchaos left a comment

Choose a reason for hiding this comment

cchaos commented Apr 14, 2021

Additional Fixes in this PR:

Global banners

cchaos commented Apr 15, 2021 • edited Loading

Reviewers:

Going forward

wylieconlon left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 15, 2021

pgayvallet left a comment

Choose a reason for hiding this comment

alvarezmelissa87 commented Apr 19, 2021

azasypkin left a comment

Choose a reason for hiding this comment

sebelga left a comment

Choose a reason for hiding this comment

ThomThomson left a comment • edited Loading

Choose a reason for hiding this comment

cchaos commented Apr 26, 2021

dominiqueclarke left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 26, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

API count

History

FrankHassanabad left a comment

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

cchaos commented Apr 13, 2021 •

edited

Loading

cchaos commented Apr 15, 2021 •

edited

Loading

ThomThomson left a comment •

edited

Loading