Fix chromeless NP apps not using full page width #54550
Conversation
pgayvallet
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
| <div className="application">{appUi}</div> | ||
| <AppContainer classes$={chrome.getApplicationClasses$()}>{appUi}</AppContainer> |
There was a problem hiding this comment.
I fixed this in the same PR as we missed it ( the chrome applicationClasses were only properly used when rendering legacy apps), however I did not add functional tests for this, as I'm not sure what the actual usage should be. Tell me if unit test are not enough.
| @@ -127,7 +127,7 @@ export class ChromeService { | |||
| ) | |||
| ) | |||
| ); | |||
| this.isVisible$ = combineLatest(this.appHidden$, this.toggleHidden$).pipe( | |||
| this.isVisible$ = combineLatest([this.appHidden$, this.toggleHidden$]).pipe( | |||
There was a problem hiding this comment.
No, changed it because the previous signature is deprecated.
| import React from 'react'; | ||
| import { Observable } from 'rxjs'; | ||
| import useObservable from 'react-use/lib/useObservable'; | ||
| import cn from 'classnames'; |
There was a problem hiding this comment.
nit: other code in the repo uses import classnames/classNames from 'classnames';
|
|
||
| const wrapperWidth = await getAppWrapperWidth(); | ||
| const windowWidth = (await browser.getWindowSize()).width; | ||
| expect(wrapperWidth).to.eql(windowWidth); |
There was a problem hiding this comment.
I haven't tried yet, but probably it's safer to test with Percy in visual_regression setup.
There was a problem hiding this comment.
I just tried it, however it seems it needs a specific configuration and/or can't be properly used from the plugin_functional test suite, or at least I did not achieve to (the snapshot fails). As the test is pretty simple and only checks a dom element's width, I think it's acceptable for now.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* add missing conditional classes on app-wrapper and application containers * update snapshot * refactor and add unit tests for service * typo * use consistent classNames naming
* upstream/master: (26 commits) Take page offset into account too (elastic#54567) [APM] Support error.{log,exception}.stacktrace.classname (elastic#54577) Np migration tsvb route validation (elastic#51850) [ML] MML calculator enhancements for multi-metric job wizard (elastic#54573) [SIEM] Fix Inspect query 'request timestamp' value changes when curso… (elastic#54223) Fix chromeless NP apps not using full page width (elastic#54550) Remove extraneous public import to prevent failing Kibana startup (elastic#54676) [Uptime] Temporarily skip flakey tests (elastic#54675) Skip failing uptime tests Create UI for alerting and actions plugin (elastic#48959) [dev/build/sass] build stylesheets for disabled plugins too (elastic#54654) [SIEM] Use bulk actions API when updating or deleting rules (elastic#54521) Support "Deprecated" label in advanced settings (elastic#54539) [Maps] add text halo color and width style properties (elastic#53827) Service Map Data API at Runtime (elastic#54027) [SIEM] Detection Engine Create Rule Design Review #1 (elastic#54442) Skip flaky test [Canvas] Enable Embeddable maps (elastic#53971) [SIEM][Detection Engine] Increases the number or rules you can view on a single page (elastic#54628) uiSettings - use validation field for image field maxSize (elastic#54522) ...
* add missing conditional classes on app-wrapper and application containers * update snapshot * refactor and add unit tests for service * typo * use consistent classNames naming
Summary
Fix #54472
hidden-chromeclass onapp-wrapperwhen displaying a chromeless NP appchrome.getApplicationClasses$()to theapplicationcontainer as this was also missing from NP app renderingChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately