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.

Sign up for GitHub

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

Jump to bottom

[Uptime] Fix full reloads while navigating to alert/ml #73796

Merged
merged 16 commits into from
Aug 10, 2020
Merged

[Uptime] Fix full reloads while navigating to alert/ml #73796

merged 16 commits into from
Aug 10, 2020

Conversation

shahzad31
Copy link
Contributor

Summary

Fixes: #73795

Uses RedirectAppLinks component in uptime to avoid full reload while navigating to other apps.

Testing

This changes no functionality, everything should work as expected.

@shahzad31 shahzad31 changed the title fix test [Uptime] Fix full reloads while navigating to alert/ml Jul 30, 2020
@shahzad31 shahzad31 requested a review from justinkambic July 30, 2020 13:02
@shahzad31 shahzad31 marked this pull request as ready for review July 30, 2020 13:03
@shahzad31 shahzad31 requested a review from a team as a code owner July 30, 2020 13:03
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jul 30, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31 shahzad31 self-assigned this Jul 30, 2020
@shahzad31 shahzad31 added v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 30, 2020
justinkambic
justinkambic reviewed Jul 30, 2020
View reviewed changes
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.

Functionality looks good and the testing pattern is great. I did ask that we add more aria-label props to the nav components, as we should've already had those.

x-pack/plugins/uptime/public/components/common/react_router_helpers/eui_link.test.tsx Outdated Show resolved Hide resolved
x-pack/plugins/uptime/public/pages/certificates.tsx Show resolved Hide resolved
x-pack/plugins/uptime/public/pages/page_header.tsx Outdated Show resolved Hide resolved
x-pack/plugins/uptime/public/pages/settings.tsx Outdated Show resolved Hide resolved
@shahzad31 shahzad31 requested a review from justinkambic July 31, 2020 15:04
justinkambic
justinkambic reviewed Jul 31, 2020
View reviewed changes
x-pack/plugins/uptime/public/components/monitor/ml/manage_ml_job.tsx Outdated Show resolved Hide resolved
x-pack/plugins/uptime/public/components/common/react_router_helpers/eui_link.tsx Outdated
to: string;
}

export const EuiReactRouterHelper: React.FC<IEuiReactRouterProps> = ({ to, children }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm questioning this naming convention. These components aren't actually part of EUI so I am concerned that overloading their prefix could be a source of confusion.

Perhaps we can refer to them as Uptime{Helper} or Um{Helper} components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have removed the Eui prefix, i think it makes more sense now.

Copy link
Contributor

@justinkambic justinkambic Aug 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I might not have explained what I meant clearly enough - I was talking in general, we should probably avoid prefixing anything that we ourselves maintain with the Eui prefix because it overloads the meaning.

If someone sees a reference to EuiReactRouterHelper in our code they may try to find documentation that doesn't exist. Renaming it as you have below (like ReactRouterEuiHelper) might be a more effective solution. Does that seem like a good idea? cc @andrewvc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh i agree, i renamed others but i missed this. let me do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 ping me when it's ready for another look and we are otherwise in pretty good shape here I think

@shahzad31 shahzad31 requested a review from justinkambic August 3, 2020 14:16
justinkambic
justinkambic reviewed Aug 6, 2020
View reviewed changes
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.

I found one small typo we should fix and then this LGTM.

x-pack/plugins/uptime/public/components/common/react_router_helpers/link_for_eui.tsx Outdated Show resolved Hide resolved
@shahzad31 shahzad31 requested a review from justinkambic August 6, 2020 17:54
@shahzad31
Copy link
Contributor Author

@justinkambic i think this is good to go now.

justinkambic
justinkambic approved these changes Aug 10, 2020
View reviewed changes
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!

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
uptime 507 +3 504

async chunks size

id value diff baseline
uptime 1.6MB +3.0KB 1.5MB

History

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

@shahzad31 shahzad31 merged commit 244ed04 into elastic:master Aug 10, 2020
@shahzad31 shahzad31 deleted the alert-app-naviate-reload branch August 10, 2020 13:02
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Aug 10, 2020
@shahzad31 @elasticmachine
[Uptime] Fix full reloads while navigating to alert/ml (elastic#73796)
1e84c7c 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@shahzad31 shahzad31 mentioned this pull request Aug 10, 2020
Merged
shahzad31 added a commit that referenced this pull request Aug 10, 2020
@shahzad31 @elasticmachine
[7.x] [Uptime] Fix full reloads while navigating to alert/ml (#73796) (…
09a6c9d 
…#74651)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 11, 2020
@gmmorris
Merge branch 'master' into task-manager/prevent-reschedule-of-running…
87c2879 
…-task

* master: (42 commits)
  Allow any hostname for chromium proxy bypass (elastic#74693)
  [ML] ML on Kibana Management: Add ability to pass a group ID filter to job management page (elastic#74533)
  [Metrics UI] Fix No Data preview pluralization (elastic#74399)
  [Bug][Security_Solution][Telemetry] Capitalize S in macOS (elastic#74688)
  Remove karma tests  from legacy maps (elastic#74668)
  [Ingest Manager] stop creating events-* index pattern and placeholder index (elastic#74683)
  [Enterprise Search] Update the browser/document title on plugin navigation (elastic#74392)
  [visualizations] Add i18n translation for 'No results found' (elastic#74619)
  [maps] convert vector style properties to TS (elastic#74553)
  bump geckodriver binary to 0.27 (elastic#74638)
  fix: update apm agents to catch abort requests (elastic#74658)
  [Security Solution] Resolver children pagination (elastic#74603)
  add memoryStatus to df analytics page and analytics table in management (elastic#74570)
  [Ingest Manager] Allow prerelease in package version (elastic#74452)
  [App Arch]: remove legacy karma tests (elastic#74599)
  [i18n] revert reverted changes (elastic#74633)
  [Lens] Clear out all attribute properties before updating (elastic#74483)
  [Uptime] Fix full reloads while navigating to alert/ml (elastic#73796)
  Index pattern field class refactor (elastic#73180)
  [ML] Functional tests - stabilize DFA job type check (elastic#74631)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinkambic justinkambic justinkambic approved these changes

Assignees

@shahzad31 shahzad31

Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Uptime] Navigating to alert app trigger full reload
4 participants
@shahzad31 @elasticmachine @kibanamachine @justinkambic