-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Dashboard] Deangularize navbar, attempt nr. 2 #61611
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
Jenkins, test this |
Jenkins, test this |
@elasticmachine merge upstream |
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.
Left a few remarks, in some cases it's just me and I don't understand the change. The most important catch is that the filter bar isn't shown anymore in full screen mode.
@@ -503,14 +549,21 @@ export class DashboardAppController { | |||
$scope.onFiltersUpdated = filters => { | |||
// The filters will automatically be set when the queryFilter emits an update event (see below) | |||
queryFilter.setFilters(filters); | |||
updateNavBar(); | |||
}; | |||
|
|||
$scope.onQuerySaved = savedQuery => { |
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.
Do we still need all of this stuff on the scope? Seems like it's only handled in react now.
}, | ||
screenTitle: $scope.screenTitle, | ||
indexPatterns: $scope.indexPatterns, | ||
filters: $scope.model.filters, |
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.
Same with the model here - seems like it's not tied to angular anymore
config: isFullScreenMode ? undefined : $scope.topNavMenu, | ||
noPadding: isFullScreenMode, | ||
showSearchBar: !isFullScreenMode, | ||
showFilterBar: showFilterBar() && !isFullScreenMode, |
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.
This hides the filter bar in fullscreen mode - currently the filter bar is shown there. isVisible
is not the same as fullscreen mode, it's about whether the chrome is shown or not (which also happens when in embedded mode)
}; | ||
|
||
const updateStateFromSavedQuery = (savedQuery: SavedQuery) => { | ||
const savedQueryFilters = savedQuery.attributes.filters || []; | ||
const globalFilters = queryFilter.getGlobalFilters(); | ||
const allFilters = [...globalFilters, ...savedQueryFilters]; | ||
|
||
const allFilters = [...globalFilters, ...savedQueryFilters, ...queryFilter.getAppFilters()]; |
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.
What is this change for? It seems like when applying a saved query the current app filters should get cleared out (and this is also what still happens it seems).
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.
This was to fix this isssue: #55443 (review)
When there is a saved query, and a filter is applied afterwards, updateStateFromSavedQuery
is triggered after the filters are applied, so savedQuery filters override the newly applied filters.
@@ -114,15 +114,6 @@ class SearchBarUI extends Component<SearchBarProps, State> { | |||
query: nextProps.query.query, | |||
language: nextProps.query.language, | |||
}; | |||
} else if ( |
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'm assuming this logic moved to https://github.com/elastic/kibana/pull/61611/files#diff-6fce6ef7c306a45fc9b878e0bcae061bR356 - what was the reason for this change?
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.
yes
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.
Can you clarify why you moved it over?
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.
Ah, sorry, missed the question. This whole logic of setting state from props is very convoluted. We have a query and a saved query, which from the point of this component is basically a query (it goes in the query bar). Then there is props, nextProps, prevState.currentProps... 🤯 In my original PR I wanted to get rid of this logic altogether and move some of this in componentDidMount
, but that was another can of worms.
If I remember correctly, I got rid of this else
bit, cause things seem to work fine without it and it's less convoluted without it. I don't think it's related to clearing up savedQuery
, that was another issue.
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.
Thanks for the explanation - I thought this was fixing some kind of buggy behavior and couldn't figure it out. I agree that it's easier to follow this way.
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.
A saved query is entirely different from a query
.
I know that's very bad naming, but a saved query is a name for filters + query string + timefilter, while a query
is just a query string
+ query language
configuration.
So moving this to the saved query logic is definitely not doing the same thing. Would you mind exploring this in a separate PR?
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 understand query string is different from saved query, but from the point of view of QueryStringInput
component it is the same - ie that component is only interested in query string
+ language
.
@elasticmachine merge upstream |
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.
Fullscreen/Embedded mode work fine for me, are you still planning to get rid of $scope
in this PR?
@flash1293 I cleaned up some of it already. I have some merging conflicts to resolve. Looks like |
@elasticmachine merge upstream |
Hey @lizozom and @flash1293 this is ready for another review. I followed Liza's suggestion to use |
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.
At the moment, if you select a saved query then refresh - the saved query id gets lost.
If this wasn't the way before, and to be consistent with other apps - you need to implement this for the time being (I'll add this to top nav at some point).
Checkout discover's updateSavedQueryId
.
Otherwise, this looks great!
Thanks @lizozom , I added the missing behavior. I also added the functional test for saved query component inside the dashboard. It's basically copied from discover, but I still think it's useful. |
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.
LGTM once green!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
@@ -965,9 +981,6 @@ export class DashboardAppController { | |||
if (outputSubscription) { | |||
outputSubscription.unsubscribe(); | |||
} | |||
if (dashboardContainer) { | |||
dashboardContainer.destroy(); |
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.
@majagrubic, sorry for following up on merged pr.
I wonder if this was moved somewhere or called somewhere else implicitly?
Otherwise it seems like moving between dashboards or navigating between dashboard / discover / visualize without full page reload would cause memory leaks and probably other undesired effects, since subscriptions within container are not cleaned up?
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 put up this tiny pr: #64154
hope this should address underlying issue. Will see if tests pass 🙏
* master: (29 commits) [Dashboard] Deangularize navbar, attempt nr. 2 (elastic#61611) refactor action filter creation utils (elastic#62969) Refresh index pattern list before redirecting (elastic#63329) [APM]fixing custom link unit tests (elastic#64045) [Ingest] EPM & Fleet are enabled when Ingest is enabled (elastic#64103) [Alerting] Fixed bug with no possibility to edit the index name after adding (elastic#64033) [Maps] Map settings: min and max zoom (elastic#63714) [kbn-storybook] Use raw loader for text files (elastic#64108) [EPM] /packages/{package} endpoint to support upgrades (elastic#63629) [SIEM] New Platform Saved Objects Registration (elastic#64029) [Endpoint] Hook to handle events needing navigation via Router (elastic#63863) Fixed small issue in clone functionality (elastic#64085) [Endpoint]EMT-146: use ingest agent for status info (elastic#63921) [SIEM] Server NP Followup (elastic#64010) Register uiSettings on New Platform (elastic#64015) [Reporting] Integration polling config with client code (elastic#63754) [Docs]7.7 SIEM doc updates (elastic#63951) [SIEM] [Cases] Tags suggestions (elastic#63878) Include datasource UUID in agent config yaml, adjust overflow height of yaml view (elastic#64027) [DOCS] Add file size setting for Data Visualizer (elastic#64006) ...
* Attempt at deangularization, nr.2 * Remove padding in fullscreen * Fixing failing functional test * Fixing remaining functional test * Fixing typescript errors * Fixing filter bar not being visible in fullscreen * Fixing filter bar not being visible in fullscreen * Rebasing against master * Fixing a small leftover * Fix order of functions * Fixing linting error * Changing noPadding to a custom class * Use filterManagers to handle filters * Rename class * Attempt at deangularization, nr.2 * Remove padding in fullscreen * Fixing failing functional test * Fixing remaining functional test * Fixing typescript errors * Fixing filter bar not being visible in fullscreen * Fixing filter bar not being visible in fullscreen * Rebasing against master * Fixing a small leftover * Fix order of functions * Fixing linting error * [APM] Agent config select box doesn't work on IE (elastic#63236) * adding value property to select options * fixing test * Use globe icon for "ext" span type on service map (elastic#63205) Both "external" and "ext" can be returned and should have the same icon. * Move shared vislib components into Charts plugin (elastic#62957) * Closes elastic#56310 Move shared vislib components into Charts plugin * Fixed imports in tests * Changed i18n IDs to match charts namespace * Renamed ColorSchemaVislibParams to ColorSchemaParams, added enums and got rid of useValidation function * Renamed ColorSchemaVislibParams to ColorSchemaParams and got rid of useValidation function * Fixed merge conflict * Replaced enums with objects again * Make uptime alert flyout test a little more resilient (elastic#62702) * [SIEM] [Cases] Unit tests for case UI components (elastic#63005) * Endpoint: Remove unused `lib` module (elastic#63248) * [Lens] Fix error in query from generated suggestion (elastic#63018) * [Lens] Fix error in query from generated suggestion * Update from review comments * Fix test Co-authored-by: Elastic Machine <[email protected]> * Resolver/node svg 2 html (elastic#62958) * Remove some SVG in Resolver nodes and replace with HTML * [Reporting] convert all server unit tests to TypeScript (elastic#62873) * [Reporting] convert all server unit tests to TypeScript * fix ts * revert unrelated change * [SIEM] Link ML Rule card CTA to license_management (elastic#63210) * Link ML Rule card CTA to license_management Taking the user directly to the license management page within kibana (where they could immediately start a trial subscription) is much more actionable than taking them to the subscriptions marketing page. * Revert translation key change Neither of these is totally accurate, and there've already been translations written for the old one. * Correctly type ILM's optional dependencies as optional (elastic#63255) And guard against their absence. * [Telemetry] use prod keys (elastic#63263) * update chromedriver dependency to 81.0.0 (elastic#63266) * task/mac-eventing-form (elastic#62999) adds mac events form for endpoint policy details Co-authored-by: oatkiller <[email protected]> * bc6 rule import april 9 (elastic#63152) * bc6 rule import april 9 Increased the lookback of the ML rules * re-import with LF chars Co-authored-by: Elastic Machine <[email protected]> * Added UI for pre-configured connectors. (elastic#63074) * Added UI for pre-configured connectors. * fixed due to comments * Fixed jest tests * Fixed due to comments and added some functional tests * test fix * Fixed failed checks * Fixed functional tests failing * TaskManager tasks scheduled without attempting to run (elastic#62078) * TaskManager tasks scheduled without attempting to run * Removing unused import Co-authored-by: Elastic Machine <[email protected]> * Changed alerting wrong param name for help xpack.encrypted_saved_objects.encryptionKey to xpack.encryptedSavedObjects.encryptionKey (elastic#63307) * fix ScopedHistory.createHref to prepend location with scoped history basePath (elastic#62407) * fix createHref to prepend with scoped history basePath + add option to exclude it. * fix prependBasePath behavior * fix test plugins urls * add pathname to endpoint url builder methods * Revert "add pathname to endpoint url builder methods" This reverts commit 7604932 * adapt createHref instead of prependBasePath * use object options for createHref * update generated doc * fixing custom link popover size and hiding scroll (elastic#63240) * Changing noPadding to a custom class * Use filterManagers to handle filters * Rename class * Applying some changes * Reverting search_bar code changes * Removing some stuff that was causing functional tests to fail * Removing refresh dashboard container which was causing errors during navigation * Do not destroy dashboardContainer * Adding updateSavedQueryId method Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Cauê Marcondes <[email protected]> Co-authored-by: Nathan L Smith <[email protected]> Co-authored-by: DianaDerevyankina <[email protected]> Co-authored-by: Brian Seeders <[email protected]> Co-authored-by: Steph Milovic <[email protected]> Co-authored-by: Robert Austin <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Brent Kimmel <[email protected]> Co-authored-by: Tim Sullivan <[email protected]> Co-authored-by: Ryland Herrick <[email protected]> Co-authored-by: CJ Cenizal <[email protected]> Co-authored-by: Ahmad Bamieh <[email protected]> Co-authored-by: Dmitry Lemeshko <[email protected]> Co-authored-by: Candace Park <[email protected]> Co-authored-by: The SpaceCake Project <[email protected]> Co-authored-by: Yuliia Naumenko <[email protected]> Co-authored-by: Brandon Kobel <[email protected]> Co-authored-by: Pierre Gayvallet <[email protected]>
* Attempt at deangularization, nr.2 * Remove padding in fullscreen * Fixing failing functional test * Fixing remaining functional test * Fixing typescript errors * Fixing filter bar not being visible in fullscreen * Fixing filter bar not being visible in fullscreen * Rebasing against master * Fixing a small leftover * Fix order of functions * Fixing linting error * Changing noPadding to a custom class * Use filterManagers to handle filters * Rename class * Attempt at deangularization, nr.2 * Remove padding in fullscreen * Fixing failing functional test * Fixing remaining functional test * Fixing typescript errors * Fixing filter bar not being visible in fullscreen * Fixing filter bar not being visible in fullscreen * Rebasing against master * Fixing a small leftover * Fix order of functions * Fixing linting error * [APM] Agent config select box doesn't work on IE (#63236) * adding value property to select options * fixing test * Use globe icon for "ext" span type on service map (#63205) Both "external" and "ext" can be returned and should have the same icon. * Move shared vislib components into Charts plugin (#62957) * Closes #56310 Move shared vislib components into Charts plugin * Fixed imports in tests * Changed i18n IDs to match charts namespace * Renamed ColorSchemaVislibParams to ColorSchemaParams, added enums and got rid of useValidation function * Renamed ColorSchemaVislibParams to ColorSchemaParams and got rid of useValidation function * Fixed merge conflict * Replaced enums with objects again * Make uptime alert flyout test a little more resilient (#62702) * [SIEM] [Cases] Unit tests for case UI components (#63005) * Endpoint: Remove unused `lib` module (#63248) * [Lens] Fix error in query from generated suggestion (#63018) * [Lens] Fix error in query from generated suggestion * Update from review comments * Fix test Co-authored-by: Elastic Machine <[email protected]> * Resolver/node svg 2 html (#62958) * Remove some SVG in Resolver nodes and replace with HTML * [Reporting] convert all server unit tests to TypeScript (#62873) * [Reporting] convert all server unit tests to TypeScript * fix ts * revert unrelated change * [SIEM] Link ML Rule card CTA to license_management (#63210) * Link ML Rule card CTA to license_management Taking the user directly to the license management page within kibana (where they could immediately start a trial subscription) is much more actionable than taking them to the subscriptions marketing page. * Revert translation key change Neither of these is totally accurate, and there've already been translations written for the old one. * Correctly type ILM's optional dependencies as optional (#63255) And guard against their absence. * [Telemetry] use prod keys (#63263) * update chromedriver dependency to 81.0.0 (#63266) * task/mac-eventing-form (#62999) adds mac events form for endpoint policy details Co-authored-by: oatkiller <[email protected]> * bc6 rule import april 9 (#63152) * bc6 rule import april 9 Increased the lookback of the ML rules * re-import with LF chars Co-authored-by: Elastic Machine <[email protected]> * Added UI for pre-configured connectors. (#63074) * Added UI for pre-configured connectors. * fixed due to comments * Fixed jest tests * Fixed due to comments and added some functional tests * test fix * Fixed failed checks * Fixed functional tests failing * TaskManager tasks scheduled without attempting to run (#62078) * TaskManager tasks scheduled without attempting to run * Removing unused import Co-authored-by: Elastic Machine <[email protected]> * Changed alerting wrong param name for help xpack.encrypted_saved_objects.encryptionKey to xpack.encryptedSavedObjects.encryptionKey (#63307) * fix ScopedHistory.createHref to prepend location with scoped history basePath (#62407) * fix createHref to prepend with scoped history basePath + add option to exclude it. * fix prependBasePath behavior * fix test plugins urls * add pathname to endpoint url builder methods * Revert "add pathname to endpoint url builder methods" This reverts commit 7604932 * adapt createHref instead of prependBasePath * use object options for createHref * update generated doc * fixing custom link popover size and hiding scroll (#63240) * Changing noPadding to a custom class * Use filterManagers to handle filters * Rename class * Applying some changes * Reverting search_bar code changes * Removing some stuff that was causing functional tests to fail * Removing refresh dashboard container which was causing errors during navigation * Do not destroy dashboardContainer * Adding updateSavedQueryId method Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Cauê Marcondes <[email protected]> Co-authored-by: Nathan L Smith <[email protected]> Co-authored-by: DianaDerevyankina <[email protected]> Co-authored-by: Brian Seeders <[email protected]> Co-authored-by: Steph Milovic <[email protected]> Co-authored-by: Robert Austin <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Brent Kimmel <[email protected]> Co-authored-by: Tim Sullivan <[email protected]> Co-authored-by: Ryland Herrick <[email protected]> Co-authored-by: CJ Cenizal <[email protected]> Co-authored-by: Ahmad Bamieh <[email protected]> Co-authored-by: Dmitry Lemeshko <[email protected]> Co-authored-by: Candace Park <[email protected]> Co-authored-by: The SpaceCake Project <[email protected]> Co-authored-by: Yuliia Naumenko <[email protected]> Co-authored-by: Brandon Kobel <[email protected]> Co-authored-by: Pierre Gayvallet <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Cauê Marcondes <[email protected]> Co-authored-by: Nathan L Smith <[email protected]> Co-authored-by: DianaDerevyankina <[email protected]> Co-authored-by: Brian Seeders <[email protected]> Co-authored-by: Steph Milovic <[email protected]> Co-authored-by: Robert Austin <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Brent Kimmel <[email protected]> Co-authored-by: Tim Sullivan <[email protected]> Co-authored-by: Ryland Herrick <[email protected]> Co-authored-by: CJ Cenizal <[email protected]> Co-authored-by: Ahmad Bamieh <[email protected]> Co-authored-by: Dmitry Lemeshko <[email protected]> Co-authored-by: Candace Park <[email protected]> Co-authored-by: The SpaceCake Project <[email protected]> Co-authored-by: Yuliia Naumenko <[email protected]> Co-authored-by: Brandon Kobel <[email protected]> Co-authored-by: Pierre Gayvallet <[email protected]>
…ana into task-manager/cancel-logging * 'task-manager/cancel-logging' of github.com:gmmorris/kibana: (28 commits) [Dashboard] Deangularize navbar, attempt nr. 2 (elastic#61611) refactor action filter creation utils (elastic#62969) Refresh index pattern list before redirecting (elastic#63329) [APM]fixing custom link unit tests (elastic#64045) [Ingest] EPM & Fleet are enabled when Ingest is enabled (elastic#64103) [Alerting] Fixed bug with no possibility to edit the index name after adding (elastic#64033) [Maps] Map settings: min and max zoom (elastic#63714) [kbn-storybook] Use raw loader for text files (elastic#64108) [EPM] /packages/{package} endpoint to support upgrades (elastic#63629) [SIEM] New Platform Saved Objects Registration (elastic#64029) [Endpoint] Hook to handle events needing navigation via Router (elastic#63863) Fixed small issue in clone functionality (elastic#64085) [Endpoint]EMT-146: use ingest agent for status info (elastic#63921) [SIEM] Server NP Followup (elastic#64010) Register uiSettings on New Platform (elastic#64015) [Reporting] Integration polling config with client code (elastic#63754) [Docs]7.7 SIEM doc updates (elastic#63951) [SIEM] [Cases] Tags suggestions (elastic#63878) Include datasource UUID in agent config yaml, adjust overflow height of yaml view (elastic#64027) [DOCS] Add file size setting for Data Visualizer (elastic#64006) ...
From end user pov, there are no notable changes here, so changed the label |
Summary
This is a second attempt to deangularize TopNavMenu in the Dashboard and replace it with a React component. Things are a lot more stable now. All the functional tests have passed and I fixed the issue found in my previously abandoned PR.
Updating
savedQuery
andquery
is still a bit of mess, so it's possible that things break here.Checklist
Delete any items that are not 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 renders correctly on smaller devices using a responsive layout. (You can test this in your browserFor maintainers