Skip to content

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

Merged
shahzad31 merged 16 commits intoelastic:masterfrom
shahzad31:alert-app-naviate-reload
Aug 10, 2020
Merged

[Uptime] Fix full reloads while navigating to alert/ml#73796
shahzad31 merged 16 commits intoelastic:masterfrom
shahzad31:alert-app-naviate-reload

Conversation

@shahzad31
Copy link
Copy Markdown
Contributor

@shahzad31 shahzad31 commented Jul 30, 2020

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
Copy Markdown
Contributor

elasticmachine commented Jul 30, 2020

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
Copy Markdown
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.

@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/common/react_router_helpers/eui_link.tsx Outdated
to: string;
}

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

@justinkambic justinkambic Jul 31, 2020

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
Copy Markdown
Contributor Author

@shahzad31 shahzad31 Aug 3, 2020

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
Copy Markdown
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
Copy Markdown
Contributor Author

@shahzad31 shahzad31 Aug 3, 2020

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
Copy Markdown
Contributor

@justinkambic justinkambic Aug 3, 2020

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
Copy Markdown
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.

@shahzad31 shahzad31 requested a review from justinkambic August 6, 2020 17:54
@shahzad31
Copy link
Copy Markdown
Contributor Author

shahzad31 commented Aug 6, 2020

@justinkambic i think this is good to go now.

justinkambic
justinkambic approved these changes Aug 10, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

shahzad31 commented Aug 10, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 10, 2020

💚 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