[Uptime] Move uptime actions to Header Actions Menu #100298

justinkambic · 2021-05-18T20:43:26Z

Summary

Resolves #89556.

Moves the header actions from the uptime app content into the stacked actions header provided by Kibana.

Before

After

Author checklist

Accessibility has been considered, relevant aria tags and other accessibility features implemented
Telemetry has been added where relevant
Docs have been added to this PR covering any new, changed, or removed features
I did make changes to the app that make the Overview page image out of date in the docs. Should I create a replacement image?
Testing for expected behavior is in place
- Automated tests exist to cover expected and edge case conditions
- User acceptance testing has been carried out to ensure the feature functions as expected within the context of how it will be used
- ~~[ ] Any special/edge cases that need to be manually tested must be documented~~
- Ensure the new layout works responsively (including down to small phone widths, where makes sense for the user flow, e.g. the error page when reacting to an alert)

Reviewer checklist

Accessibility has been considered, relevant aria tags and other accessibility features implemented
Telemetry has been added where relevant
Docs have been added to this PR covering any new, changed, or removed features
Testing for expected behavior is in place
- Automated tests exist to cover expected and edge case conditions
- User acceptance testing has been carried out to ensure the feature functions as expected within the context of how it will be used
- Any special/edge cases that need to be manually tested must be documented
- Ensure the new layout works responsively (including down to small phone widths, where makes sense for the user flow, e.g. the error page when reacting to an alert)

…ier.

elasticmachine · 2021-05-20T17:42:10Z

Pinging @elastic/uptime (Team:uptime)

justinkambic · 2021-05-24T12:43:17Z

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

-};
+export const ActionMenu = ({ appMountParameters }: { appMountParameters: AppMountParameters }) => (
+  <HeaderMenuPortal setHeaderActionMenu={appMountParameters.setHeaderActionMenu}>
+    <ActionMenuContent />


There weren't any test for these button controls, so I added some while I was in here. I extracted this content to a new component because we don't have much interest in testing the HeaderMenuPortal API.

justinkambic · 2021-05-24T13:16:01Z

@elasticmachine merge upstream

…Menu

dominiqueclarke

Looks great!
One thing I noticed, is that each one of the header options is wrapped in a button, but the actual action is nested inside and may be either a button or a link. This is causing some keyboard navigation and screenreader issues, as each option can be tabbed to twice.

When a button is nested inside a button, the first tab will read out the content, but when you attempt to use the keyboard for clicking it does nothing, since the wrapper button doesn't contain an onclick. Then on the second tab it rereads the content, and at that time you can use the keyboard to click the button.

For the Settings and Analyze data links however, the experience is even more difficult. The first tab once again reads the button content, but i clicking does nothing due to a lack of an onclick handler. Then once you tab again, you land on the link which is read out, but the button content and instructions for clicking the button are again read. However, since this is a link, the instructions for how to actually "press" the link are incorrect, and following the screenreader instructions will result in nothing happening. Instead, you have to press the enter key to access the link.

We may need to revisit wrapping each of these options in an empty button, and instead unnest the elements that contain the actual behavior and use some other component to maintain style consistency.

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

dominiqueclarke · 2021-05-24T13:24:30Z

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

+        <EuiToolTip position="top" content={<p>{ANALYZE_MESSAGE}</p>}>
+          <EuiButtonEmpty
+            aria-label={i18n.translate('xpack.uptime.page_header.exploratoryLink.label', {
+              defaultMessage: 'Navigate to the exploratory view to visualize data',


I believe the user facing name is current 'Analyze Data'.

Had another pass at this, how is this looking? 1c9d3e7

…Menu

justinkambic · 2021-05-25T13:51:23Z

I've addressed the smaller items; working on the nested button issue now.

shahzad31 · 2021-05-25T14:10:30Z

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

+  it('renders settings link', () => {
+    const { getByRole, getByText } = render(<ActionMenuContent />);
+
+    const settingsAnchor = getByRole('link', { name: 'Navigate to the Uptime settings page' });


i usually avoid getByRole they tends to be slow.

Was added as a response to feedback.

shahzad31 · 2021-05-25T14:13:08Z

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

+          })}
+          iconType="gear"
+        >
+          <FormattedMessage id="xpack.uptime.page_header.settingsLink" defaultMessage="Settings" />


@justinkambic @formgeist have we decided on the order of buttons? in APM settings comes before Alerts. Here alerts is before Settings, we should use same order for same kind of buttons.

@shahzad31 @justinkambic The correct order is described in the latest issue that relates to the menu options #99902

I was following the screenshot in the issue. I prefer to not have settings be the first thing in the list because I tend to not think of Settings as the most important item in nav menus, but I agree we need to have uniformity here.

@formgeist should we follow the ordering of APM for this change, and then create a new issue to switch the buttons across Observability apps? Alternatively we can just keep settings in the front.

I just pushed 452d11f which switches the settings and alerts options.

@formgeist should we follow the ordering of APM for this change, and then create a new issue to switch the buttons across Observability apps? Alternatively we can just keep settings in the front.

We have separate issues that address this for the other Observability apps.

@formgeist so you want me to revert the change and have Alerts before Settings?

@justinkambic Sorry, I didn't make that clear. Settings will go in front of alerts like the design suggested.

Ok, sounds like it's configured the way we want then.

…sting `button`s within `buttons`.

justinkambic · 2021-05-25T14:25:08Z

One thing I noticed, is that each one of the header options is wrapped in a button, but the actual action is nested inside and may be either a button or a link.

This was a great find and it's caused by a misuse of the API for EuiHeader. I've updated it to use EuiHeaderSectionItem, which wraps EuiHeaderLinks children but seems to not render a button/link around the child content. I was able to tab through each item and navigate with the keyboard.

…h APM.

justinkambic · 2021-05-25T14:37:08Z

Thanks all for feedback, I think it's all been addressed so far.

shahzad31

My feedback is addressed !!

kibanamachine · 2021-05-25T16:14:52Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`uptime`	612	613	+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.1MB	1.1MB	+978.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`canvas`	29	25	-4
`crossClusterReplication`	8	6	-2
`fleet`	22	20	-2
`globalSearch`	4	2	-2
`indexManagement`	12	7	-5
`infra`	261	149	-112
`lens`	67	45	-22
`licensing`	18	15	-3
`lists`	239	236	-3
`maps`	286	208	-78
`ml`	121	115	-6
`monitoring`	109	56	-53
`securitySolution`	386	342	-44
`stackAlerts`	101	95	-6
total			-342

History

💚 Build #126882 succeeded e5ef95f
💚 Build #126754 succeeded c6f4e54
💚 Build #126624 succeeded 54d981b7c35d1274dd6e4622b7bdac1e3afe1408
💚 Build #126459 succeeded 5461e5d6fe334525d4003aab4d1f5e0bf1f72dcb

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

cc @justinkambic

dominiqueclarke

LGTM. I created #100725 as I noticed some potential focus issues related to this but broader than this PR.

* Move uptime actions to Kibana's HeaderActionsMenu. * Delete a comment. * Extract ActionMenu content to dedicated component to make testing easier. * Add tests. * Use `EuiHeaderLinks` instead of `EuiFlexItem`. * Clean up tests. * Prefer `getByRole` for a test. * Fix copy mistake. * Fix a test broken by the previous commit. * Prefer `EuiHeaderSectionItem` over `EuiHeaderSectionLink` to avoid nesting `button`s within `buttons`. * Reverse "Settings" and "Alerts" menu options to make them uniform with APM. Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-05-26T19:34:03Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* Move uptime actions to Kibana's HeaderActionsMenu. * Delete a comment. * Extract ActionMenu content to dedicated component to make testing easier. * Add tests. * Use `EuiHeaderLinks` instead of `EuiFlexItem`. * Clean up tests. * Prefer `getByRole` for a test. * Fix copy mistake. * Fix a test broken by the previous commit. * Prefer `EuiHeaderSectionItem` over `EuiHeaderSectionLink` to avoid nesting `button`s within `buttons`. * Reverse "Settings" and "Alerts" menu options to make them uniform with APM. Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Justin Kambic <[email protected]>

justinkambic self-assigned this May 18, 2021

justinkambic added release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0 v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed labels May 18, 2021

justinkambic force-pushed the 89556/move-header-actions-to-HeaderActionsMenu branch from 6f53c56 to 54d981b Compare May 19, 2021 19:23

justinkambic added 6 commits May 20, 2021 13:26

Move uptime actions to Kibana's HeaderActionsMenu.

deb794c

Delete a comment.

85ef425

Extract ActionMenu content to dedicated component to make testing eas…

2010636

…ier.

Add tests.

b4a7ba4

Use EuiHeaderLinks instead of EuiFlexItem.

62d93ff

Clean up tests.

c6f4e54

justinkambic force-pushed the 89556/move-header-actions-to-HeaderActionsMenu branch from 54d981b to c6f4e54 Compare May 20, 2021 17:36

justinkambic marked this pull request as ready for review May 20, 2021 17:42

justinkambic requested a review from a team as a code owner May 20, 2021 17:42

justinkambic commented May 24, 2021

View reviewed changes

Merge branch 'master' into 89556/move-header-actions-to-HeaderActions…

e5ef95f

…Menu

dominiqueclarke requested changes May 24, 2021

View reviewed changes

justinkambic added 3 commits May 25, 2021 09:36

Merge branch 'master' into 89556/move-header-actions-to-HeaderActions…

4342caa

…Menu

Prefer getByRole for a test.

6fc9be1

Fix copy mistake.

1c9d3e7

shahzad31 reviewed May 25, 2021

View reviewed changes

shahzad31 requested changes May 25, 2021

View reviewed changes

justinkambic added 2 commits May 25, 2021 10:20

Fix a test broken by the previous commit.

475ba5d

Prefer EuiHeaderSectionItem over EuiHeaderSectionLink to avoid ne…

8a0e37e

…sting `button`s within `buttons`.

Reverse "Settings" and "Alerts" menu options to make them uniform wit…

452d11f

…h APM.

shahzad31 approved these changes May 25, 2021

View reviewed changes

dominiqueclarke approved these changes May 26, 2021

View reviewed changes

justinkambic merged commit b189d05 into elastic:master May 26, 2021

justinkambic deleted the 89556/move-header-actions-to-HeaderActionsMenu branch May 26, 2021 19:31

kibanamachine mentioned this pull request May 26, 2021

[7.x] [Uptime] Move uptime actions to Header Actions Menu (#100298) #100728

Merged

justinkambic mentioned this pull request Jun 1, 2021

[Uptime] Add Settings and Alerts to the menu actions in the Kibana header bar #99902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime] Move uptime actions to Header Actions Menu #100298

[Uptime] Move uptime actions to Header Actions Menu #100298

justinkambic commented May 18, 2021 •

edited

Loading

elasticmachine commented May 20, 2021

justinkambic May 24, 2021

justinkambic commented May 24, 2021

dominiqueclarke left a comment •

edited

Loading

dominiqueclarke May 24, 2021

justinkambic May 25, 2021

justinkambic commented May 25, 2021

shahzad31 May 25, 2021

justinkambic May 25, 2021

shahzad31 May 25, 2021 •

edited

Loading

shahzad31 May 25, 2021

formgeist May 25, 2021

justinkambic May 25, 2021

justinkambic May 25, 2021

formgeist May 25, 2021

justinkambic May 25, 2021

formgeist May 26, 2021

justinkambic May 26, 2021

justinkambic commented May 25, 2021

justinkambic commented May 25, 2021

shahzad31 left a comment

kibanamachine commented May 25, 2021

References to deprecated APIs

dominiqueclarke left a comment

kibanamachine commented May 26, 2021

[Uptime] Move uptime actions to Header Actions Menu #100298

[Uptime] Move uptime actions to Header Actions Menu #100298

Conversation

justinkambic commented May 18, 2021 • edited Loading

Summary

Before

After

Author checklist

Reviewer checklist

elasticmachine commented May 20, 2021

Choose a reason for hiding this comment

justinkambic commented May 24, 2021

dominiqueclarke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinkambic commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzad31 May 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinkambic commented May 25, 2021

justinkambic commented May 25, 2021

shahzad31 left a comment

Choose a reason for hiding this comment

kibanamachine commented May 25, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

References to deprecated APIs

History

dominiqueclarke left a comment

Choose a reason for hiding this comment

kibanamachine commented May 26, 2021

💚 Backport successful

justinkambic commented May 18, 2021 •

edited

Loading

dominiqueclarke left a comment •

edited

Loading

shahzad31 May 25, 2021 •

edited

Loading