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.

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

[Uptime] Fix/issue 48 integration popup closes after refresh #45759

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Sep 16, 2019

Summary

Resolves elastic/uptime#48

In monitors list, this PR will keep integrations popover open after auto refresh happens, keep in mind if user click refresh manually, this will close the popover, since that is normal behavior.

Checklist

Testing:

  1. Open Uptime app, make sure it has some monitors
  2. In Monitors list , click action button in front of any monitor
  3. Popover with integrations links will open
  4. Wait for default refresh interval or reduce interval by typing number in url autoRefresh param
  5. Once refresh happens, popover should remain open.

This is the popover

image

@shahzad31 shahzad31 marked this pull request as ready for review September 16, 2019 09:48
@shahzad31 shahzad31 added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Sep 16, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

LGTM - had a few subjective comments/nit things. Aliasing the provider name is the only suggested change I'm really attached to.

Functionally this works really well!
image
I have subsequent refresh requests with the popover still open, so excellent work there.

This is a really clean implementation of Redux and was a great refresher for me. I think we should have an ongoing initiative to extract additional UI state as opportunities present themselves.

import { rootReducer } from './reducers';
import { rootSaga } from './sagas';

const composeEnhancers = window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something I'm unfamiliar with, so I'm sorry if this question is a little basic, but this means that __REDUX_DEVTOOLS_EXTENSION_COMPOSE__ will be undefined on window for non-dev builds? It seems like this is the recommended way to configure compose per the advanced store setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it's checking if browser has redux dev tools extension installed, it will use compose available from extension if it is available, otherwise just compose.

x-pack/legacy/plugins/uptime/public/uptime_app.tsx Outdated Show resolved Hide resolved
@shahzad31 shahzad31 requested review from a team as code owners September 17, 2019 11:25
@legrego legrego removed the request for review from a team September 17, 2019 12:10
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@shahzad31 shahzad31 merged commit a204a43 into elastic:master Sep 17, 2019
@shahzad31 shahzad31 deleted the fix/issue-48-integration-popup-closes-after-refresh branch September 17, 2019 17:15
@shahzad31 shahzad31 self-assigned this Sep 17, 2019
rylnd added a commit to rylnd/kibana that referenced this pull request Sep 17, 2019
* master: (33 commits)
  [easy] Exclude __examples__ from coverage (elastic#45556)
  [DOCS] Update CCR links (elastic#44012)
  Use unique junit report filenames again (elastic#45897)
  [ftr/savedObjects] add simple saved object api client to ftr s… (elastic#45856)
  New visualization editor Lens (elastic#36437)
  Sort using unix timestamp value (elastic#43162)
  [APM] Use POST instead of implicit GET (elastic#45903)
  [Canvas] Converting workpad header components to typescript and adding i18n (elastic#45274)
  skip flaky test (elastic#45884)
  set IS_PIPELINE_JOB in intake jobs (elastic#45850)
  [Uptime] Fix/issue 48 integration popup closes after refresh (elastic#45759)
  [Logs UI] Support zoom by brushing in the log rate chart (elastic#45879)
  [DOCS] Changes name to host (elastic#45798)
  [ML] Add population job wizard test (elastic#45765)
  [skip-ci][Maps][File upload] Geojson indexing and styling docs (elastic#41394)
  remove setTimeoue for state change (elastic#45853)
  [Graph] Restructure folders and add readme (elastic#45782)
  [ML] Enhance job id error message (elastic#45349)
  [SIEM] Do not update state component when they did unmount (elastic#45847)
  [i18n] sync from 7.4 latest translations (elastic#45823)
  ...
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Sep 18, 2019
…#45759)

* integrate redux into uptime app

* update integrations popup handling

* keep popover open after page refresh

* updated unit test snaps

* update unit tests

* fixed types
shahzad31 added a commit that referenced this pull request Sep 18, 2019
…#45999)

* integrate redux into uptime app

* update integrations popup handling

* keep popover open after page refresh

* updated unit test snaps

* update unit tests

* fixed types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Integrations popover does not remain open when app refreshes
5 participants