[Uptime] Fix/details page tabs #86296

dominiqueclarke · 2020-12-17T14:27:52Z

Summary

Fixes: #85826
Fixes: #86743
Fixes: #86174

Refactors PageHeader component to rely on props
Removes tabs from detail monitor pages
Uses the monitor url as the Monitor title when the name is undefined and the id is auto generated

To test

Certificates header renders appropriately, with tabs and cert refresh button
Settings header renders appropriately, with tabs
Monitor header renders appropriately, without tabs and with date picker
Overview header renders appropriately, with tabs and with date picker
Step details page renders without a header

Overview Page Heading

Monitor Page Heading

Certificates Page Header

Settings Page Header

Waterfall Page Heading

elasticmachine · 2020-12-17T14:27:54Z

Pinging @elastic/uptime (Team:uptime)

dominiqueclarke · 2020-12-17T17:13:32Z

@elasticmachine merge upstream

…nitor names and auto generated monitor ids

dominiqueclarke · 2020-12-17T20:04:43Z

@elasticmachine merge upstream

x-pack/plugins/uptime/public/components/monitor/monitor_title.tsx

dominiqueclarke · 2020-12-22T14:12:49Z

@elasticmachine merge upstream

andrewvc · 2020-12-22T23:39:10Z

x-pack/plugins/uptime/public/components/common/header/__tests__/page_header.test.tsx

+      history
+    );
+    expect(component.find('h1').text()).toBe(monitorName);
+    expect(component.find('[data-test-subj="uptimeSettingsToOverviewLink"]').length).toBe(0);


Is there a way we can just add a data-test-subj to the whole tab group instead of looking for specific links? Seems like the wrong approach, and fragile if the links themselves change.

andrewvc · 2020-12-22T23:41:11Z

x-pack/plugins/uptime/public/components/common/header/page_header.tsx

-        <EuiFlexItem>
-          <PageTabs />
-        </EuiFlexItem>
+        <EuiFlexItem>{isMonitorRoute ? <MonitorPageTitle /> : <PageTabs />}</EuiFlexItem>


It's a bit weird to me that the header should care about what page it's on. It would make more sense, IMHO, for the page to tell the header what should be in the header, that would, IMHO be less fragile. Each page could just say explicitly "I want this type of header".

Thinking ahead here, when we add a new page we want whoever works on it to make an explicit choice of header.

andrewvc · 2020-12-22T23:44:08Z

x-pack/plugins/uptime/public/components/monitor/__tests__/monitor_title.test.tsx

+
+jest.mock('react-router-dom', () => ({
+  __esModule: true,
+  // @ts-ignore


What's the reason for this being ignored? Any type script ignore statements should come with a comment explaining the why here. As a reviewer (or someone coming along later) it answers a lot of questions.

andrewvc · 2020-12-22T23:51:36Z

x-pack/plugins/uptime/public/components/monitor/__tests__/monitor_title.test.tsx

+  const url = {
+    full: 'https://www.elastic.co/',
+  };
+  const monitorStatus = {


It feels like the complexity of these mocks at this point justifies a bit of refactoring here.

We do things like this elsewhere, but it's something we should avoid when we can. The reason is, if we add any additional mandatory fields to this type, this is one more file we must update. Also, it's hard to read etc.

I am wondering if we could make a new selector in redux that just pulls the fields we need, so we would only need to mock that minimal set of fields. In general, when passing a complex mocked object, that can point to a need to reduce the surface area of the function (or component) being tested. It may accept a more complex object than it needs to. I'm also open to other approaches that could simplify the tests here.

x-pack/plugins/uptime/public/components/monitor/monitor_title.tsx

dominiqueclarke · 2021-01-04T17:22:01Z

I am fine with the refactored appraoch, but it causes few issues

this means whole tabs component will be re-rendered in each page, which means the focus like we see now

is gone if we switch tabs

Good catch! I'm thinking to resolve this issue, while still keeping the PageHeader component prop based, to move the PageHeader component back to routes.tsx, and pass the props for each route through the route object.

dominiqueclarke · 2021-01-04T21:44:57Z

I am fine with the refactored appraoch, but it causes few issues

this means whole tabs component will be re-rendered in each page, which means the focus like we see now

is gone if we switch tabs

My original thought of how to resolve this was misguided, as it still causes the tabs component to be mounted, unmounted, and remounted, causing loss of focus. We could focus on the current tab on mount, but that would cause an odd experience when a user visits the page outside of using the tabs, for example visiting the direct url. @shahzad31's original implementation tracking the routes is likely the best approach given these challenges.

dominiqueclarke · 2021-01-05T16:26:22Z

@elasticmachine merge upstream

shahzad31 · 2021-01-06T17:13:57Z

x-pack/plugins/uptime/public/components/monitor/__tests__/monitor_title.test.tsx

+jest.mock('react-redux', () => {
+  const originalModule = jest.requireActual('react-redux');
+
+  return {
+    ...originalModule,
+    useSelector: jest.fn(),
+  };
+});


i wonder if it make sense to use mocks we have in file

x-pack/plugins/uptime/public/lib/helper/test_helpers.ts

Thanks! This works great. I tried to create one for react-router-dom too, but it wouldn't let me use a spy without first running jest.mock in the test file, making a reusable function less helpful.

…outerDomHooks

…larke/kibana into fix/details-page-tabs

shahzad31 · 2021-01-07T16:30:42Z

x-pack/plugins/uptime/public/components/common/header/page_header.tsx

      </EuiFlexGroup>
-      {isMonRoute && <EuiHorizontalRule margin="m" />}
-      {!isMonRoute && <EuiSpacer size="m" />}


i believe this includeSpacer prop isn't needed, can we move this perhaps to top of those pages? where it's needed?

shahzad31

Please address the includeSpacer stuff, otherwise it's looking good !!

dominiqueclarke · 2021-01-10T23:13:37Z

@elasticmachine merge upstream

kibanamachine · 2021-01-11T01:30:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bae3e91

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`uptime`	567	568	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`uptime`	1.0MB	1.0MB	+193.0B

History

💚 Build #97630 succeeded f294605
💚 Build #97476 succeeded 8aae9e0
💔 Build #97473 failed 59a59ccb6eb4b9e01eed3261187482294afba654
💔 Build #97470 failed 13395fc4a876c4612f9ae0556ab0315b14b5a6d5
💔 Build #97467 failed cbca6e9

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

* Remove tabs from details page * update * fix monitord id * var name * add Uptime PageHeader tests to test for the presences of tabs or header * add Uptime MonitorPageTitle test * Uptime adjust auto generated monitor id regex * Uptime add tests for MonitorPageTitle to test behavior for missing monitor names and auto generated monitor ids * remove history from MonitorPageTitle test * adjust uptime tabs tests * adjust MonitorPageTitle tests to mock useSelector * adjust uptime PageHeader tests * adjust import order in page_header.test * add props to Uptime PageHeader to determine render, rather than route context * alphabetize props in Uptime PageHeader * remove header from individual pages * add indepdent page header route that matches all paths * adjust monitor tests to use mockReduxHooks helper, and add mockReactRouterDomHooks * update tests * adjust header spacing Co-authored-by: Shahzad <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

* master: [APM] Define placement “Right” to offset tooltip (elastic#87729) Fix UI glitch on SOM delete confirmation modal (elastic#87623) Remove src/plugins/vis_default_editor -> src/plugins/visualizations cyclic dependencies (elastic#86988) [Timelion] Fix tests flakiness on suggestion click (elastic#87273) [Uptime] Fix/details page tabs (elastic#86296) [ML] Fix earliest and latest texts for date fields (elastic#87482) chore(NA): move grokdebugger plugin test fixtures out of __tests__ folder (elastic#87765) [Security Solution] Refactor Cypress scenarios to use internal contex… (elastic#86609) [Security Solution] Unskip cypress tests (elastic#86653)

* Remove tabs from details page * update * fix monitord id * var name * add Uptime PageHeader tests to test for the presences of tabs or header * add Uptime MonitorPageTitle test * Uptime adjust auto generated monitor id regex * Uptime add tests for MonitorPageTitle to test behavior for missing monitor names and auto generated monitor ids * remove history from MonitorPageTitle test * adjust uptime tabs tests * adjust MonitorPageTitle tests to mock useSelector * adjust uptime PageHeader tests * adjust import order in page_header.test * add props to Uptime PageHeader to determine render, rather than route context * alphabetize props in Uptime PageHeader * remove header from individual pages * add indepdent page header route that matches all paths * adjust monitor tests to use mockReduxHooks helper, and add mockReactRouterDomHooks * update tests * adjust header spacing Co-authored-by: Shahzad <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Shahzad <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

shahzad31 and others added 7 commits December 15, 2020 14:53

Remove tabs from details page

ccf1107

Merge branch 'master' into details-page-tabs

1b9bce4

update

12b3303

fix monitord id

b8977ca

Merge branch 'master' into details-page-tabs

c6ea0a6

var name

914f9b3

add Uptime PageHeader tests to test for the presences of tabs or header

0b9a3b2

dominiqueclarke added v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 17, 2020

dominiqueclarke requested a review from a team as a code owner December 17, 2020 14:27

dominiqueclarke added the bug Fixes for quality problems that affect the customer experience label Dec 17, 2020

dominiqueclarke changed the title ~~Fix/details page tabs~~ [Uptime] Fix/details page tabs Dec 17, 2020

add Uptime MonitorPageTitle test

57439d6

dominiqueclarke added 2 commits December 17, 2020 15:02

Uptime adjust auto generated monitor id regex

b157dae

Uptime add tests for MonitorPageTitle to test behavior for missing mo…

1792f21

…nitor names and auto generated monitor ids

dominiqueclarke force-pushed the fix/details-page-tabs branch from 0e31d33 to 1792f21 Compare December 17, 2020 20:04

kibanamachine and others added 2 commits December 17, 2020 15:04

Merge branch 'master' into fix/details-page-tabs

5df147c

remove history from MonitorPageTitle test

b1df186

dominiqueclarke commented Dec 17, 2020

View reviewed changes

x-pack/plugins/uptime/public/components/monitor/monitor_title.tsx Show resolved Hide resolved

dominiqueclarke requested a review from andrewvc December 17, 2020 20:23

Merge branch 'master' into fix/details-page-tabs

93a3441

andrewvc reviewed Dec 22, 2020

View reviewed changes

dominiqueclarke added 2 commits December 22, 2020 20:09

adjust uptime tabs tests

fd703a6

adjust MonitorPageTitle tests to mock useSelector

34cbb18

dominiqueclarke added 2 commits January 5, 2021 10:49

remove header from individual pages

9bac833

add indepdent page header route that matches all paths

104320f

Merge branch 'master' into fix/details-page-tabs

7e6de88

shahzad31 reviewed Jan 6, 2021

View reviewed changes

dominiqueclarke added 3 commits January 6, 2021 20:32

adjust monitor tests to use mockReduxHooks helper, and add mockReactR…

d884b92

…outerDomHooks

Merge branch 'fix/details-page-tabs' of https://github.com/dominiquec…

0791a25

…larke/kibana into fix/details-page-tabs

pull upstream master and resolve conflicts

cbca6e9

dominiqueclarke force-pushed the fix/details-page-tabs branch 2 times, most recently from 06f4d7a to 59a59cc Compare January 7, 2021 02:39

update tests

8aae9e0

dominiqueclarke force-pushed the fix/details-page-tabs branch from 59a59cc to 8aae9e0 Compare January 7, 2021 03:22

shahzad31 reviewed Jan 7, 2021

View reviewed changes

shahzad31 approved these changes Jan 7, 2021

View reviewed changes

adjust header spacing

f294605

dominiqueclarke force-pushed the fix/details-page-tabs branch from ca076cb to f294605 Compare January 7, 2021 17:46

Merge branch 'master' into fix/details-page-tabs

bae3e91

dominiqueclarke merged commit 0ff6965 into elastic:master Jan 11, 2021

dominiqueclarke deleted the fix/details-page-tabs branch January 11, 2021 01:55

This was referenced Jan 11, 2021

[7.x] [Uptime] Fix/details page tabs (#86296) #87795

Merged

[7.11] [Uptime] Fix/details page tabs (#86296) #87796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime] Fix/details page tabs #86296

[Uptime] Fix/details page tabs #86296

dominiqueclarke commented Dec 17, 2020 •

edited

Loading

elasticmachine commented Dec 17, 2020

dominiqueclarke commented Dec 17, 2020

dominiqueclarke commented Dec 17, 2020

dominiqueclarke commented Dec 22, 2020

andrewvc Dec 22, 2020

andrewvc Dec 22, 2020

andrewvc Dec 22, 2020

andrewvc Dec 22, 2020

dominiqueclarke commented Jan 4, 2021 •

edited

Loading

dominiqueclarke commented Jan 4, 2021 •

edited

Loading

dominiqueclarke commented Jan 5, 2021

shahzad31 Jan 6, 2021

dominiqueclarke Jan 7, 2021

shahzad31 Jan 7, 2021

shahzad31 left a comment

dominiqueclarke commented Jan 10, 2021

kibanamachine commented Jan 11, 2021

[Uptime] Fix/details page tabs #86296

[Uptime] Fix/details page tabs #86296

Conversation

dominiqueclarke commented Dec 17, 2020 • edited Loading

Summary

elasticmachine commented Dec 17, 2020

dominiqueclarke commented Dec 17, 2020

dominiqueclarke commented Dec 17, 2020

dominiqueclarke commented Dec 22, 2020

andrewvc Dec 22, 2020

Choose a reason for hiding this comment

andrewvc Dec 22, 2020

Choose a reason for hiding this comment

andrewvc Dec 22, 2020

Choose a reason for hiding this comment

andrewvc Dec 22, 2020

Choose a reason for hiding this comment

dominiqueclarke commented Jan 4, 2021 • edited Loading

dominiqueclarke commented Jan 4, 2021 • edited Loading

dominiqueclarke commented Jan 5, 2021

shahzad31 Jan 6, 2021

Choose a reason for hiding this comment

dominiqueclarke Jan 7, 2021

Choose a reason for hiding this comment

shahzad31 Jan 7, 2021

Choose a reason for hiding this comment

shahzad31 left a comment

Choose a reason for hiding this comment

dominiqueclarke commented Jan 10, 2021

kibanamachine commented Jan 11, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

dominiqueclarke commented Dec 17, 2020 •

edited

Loading

dominiqueclarke commented Jan 4, 2021 •

edited

Loading

dominiqueclarke commented Jan 4, 2021 •

edited

Loading