Skip to content

[APM] Divide "Actions menu" into sections to improve readability - #56623

Merged
cauemarcondes merged 16 commits into
elastic:masterfrom
cauemarcondes:actions-menu
Feb 17, 2020
Merged

cauemarcondes merged 16 commits into
elastic:masterfrom
cauemarcondes:actions-menu

Conversation

@cauemarcondes

Copy link
Copy Markdown
Contributor

closes #53588

Screenshot 2020-02-03 at 15 11 33

Screenshot 2020-02-03 at 15 20 58

Screenshot 2020-02-03 at 15 21 15

@dgieselaar dgieselaar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking great, just a few suggestions/qs.

Comment thread x-pack/plugins/translations/translations/zh-CN.json Outdated
Comment thread x-pack/legacy/plugins/apm/public/context/ApmPluginContext.tsx Outdated
@smith
smith self-requested a review February 5, 2020 15:53

@smith smith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tenor-196430137

@cauemarcondes

Copy link
Copy Markdown
Contributor Author

jenkins, retest this please

@cauemarcondes

Copy link
Copy Markdown
Contributor Author

retest

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed


Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__.Transaction action menu shows required sections only

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toEqual(expected) // deep equality

- Expected
+ Received

@@ -2,11 +2,11 @@
    Array [
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/logs?time=1580943600000&filter=trace.id:%22123%22%20OR%20123",
+           "href": "#/link-to/logs?time=1580947200000&filter=trace.id:%22123%22%20OR%20123",
            "key": "traceLogs",
            "label": "Trace logs",
          },
        ],
        "key": "traceDetails",
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__/sections.test.ts:30:7)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__.Transaction action menu shows pod and required sections only

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toEqual(expected) // deep equality

- Expected
+ Received

@@ -2,11 +2,11 @@
    Array [
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/pod-logs/123?time=1580943600000",
+           "href": "#/link-to/pod-logs/123?time=1580947200000",
            "key": "podLogs",
            "label": "Pod logs",
          },
          Object {
            "condition": true,
@@ -21,11 +21,11 @@
      },
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/logs?time=1580943600000&filter=trace.id:%22123%22%20OR%20123",
+           "href": "#/link-to/logs?time=1580947200000&filter=trace.id:%22123%22%20OR%20123",
            "key": "traceLogs",
            "label": "Trace logs",
          },
        ],
        "key": "traceDetails",
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__/sections.test.ts:78:7)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__.Transaction action menu shows host and required sections only

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toEqual(expected) // deep equality

- Expected
+ Received

@@ -2,11 +2,11 @@
    Array [
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/host-logs/foo?time=1580943600000",
+           "href": "#/link-to/host-logs/foo?time=1580947200000",
            "key": "hostLogs",
            "label": "Host logs",
          },
          Object {
            "condition": true,
@@ -21,11 +21,11 @@
      },
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/logs?time=1580943600000&filter=trace.id:%22123%22%20OR%20123",
+           "href": "#/link-to/logs?time=1580947200000&filter=trace.id:%22123%22%20OR%20123",
            "key": "traceLogs",
            "label": "Trace logs",
          },
        ],
        "key": "traceDetails",
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__/sections.test.ts:145:7)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

History

  • 💔 Build #24899 failed af1123e438e40c9a2c4faeae68d6335ea3323710
  • 💚 Build #24536 succeeded c827ed1bfc7c3c0dd10a08e236b66f11f9bec8ad
  • 💚 Build #24525 succeeded 64ccbf2d8a5d781d8d0a94b10cc7e55643e14a79
  • 💚 Build #24051 succeeded de1a0b5e02ddf0423934ece00ccb45943995c9d4
  • 💔 Build #24015 failed 1805232a0943138eee6c719cf4ccb58c758d0451

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

@elastic elastic deleted a comment from kibanamachine Feb 7, 2020
@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed


Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__.Transaction action menu shows required sections only

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toEqual(expected) // deep equality

- Expected
+ Received

@@ -2,11 +2,11 @@
    Array [
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/logs?time=1580979600000&filter=trace.id:%22123%22%20OR%20123",
+           "href": "#/link-to/logs?time=1580983200000&filter=trace.id:%22123%22%20OR%20123",
            "key": "traceLogs",
            "label": "Trace logs",
          },
        ],
        "key": "traceDetails",
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__/sections.test.ts:35:7)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__.Transaction action menu shows pod and required sections only

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toEqual(expected) // deep equality

- Expected
+ Received

@@ -2,11 +2,11 @@
    Array [
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/pod-logs/123?time=1580979600000",
+           "href": "#/link-to/pod-logs/123?time=1580983200000",
            "key": "podLogs",
            "label": "Pod logs",
          },
          Object {
            "condition": true,
@@ -21,11 +21,11 @@
      },
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/logs?time=1580979600000&filter=trace.id:%22123%22%20OR%20123",
+           "href": "#/link-to/logs?time=1580983200000&filter=trace.id:%22123%22%20OR%20123",
            "key": "traceLogs",
            "label": "Trace logs",
          },
        ],
        "key": "traceDetails",
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__/sections.test.ts:83:7)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__.Transaction action menu shows host and required sections only

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toEqual(expected) // deep equality

- Expected
+ Received

@@ -2,11 +2,11 @@
    Array [
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/host-logs/foo?time=1580979600000",
+           "href": "#/link-to/host-logs/foo?time=1580983200000",
            "key": "hostLogs",
            "label": "Host logs",
          },
          Object {
            "condition": true,
@@ -21,11 +21,11 @@
      },
      Object {
        "actions": Array [
          Object {
            "condition": true,
-           "href": "#/link-to/logs?time=1580979600000&filter=trace.id:%22123%22%20OR%20123",
+           "href": "#/link-to/logs?time=1580983200000&filter=trace.id:%22123%22%20OR%20123",
            "key": "traceLogs",
            "label": "Trace logs",
          },
        ],
        "key": "traceDetails",
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/shared/TransactionActionMenu/__test__/sections.test.ts:150:7)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

History

  • 💔 Build #25170 failed af1123e438e40c9a2c4faeae68d6335ea3323710
  • 💔 Build #24899 failed af1123e438e40c9a2c4faeae68d6335ea3323710
  • 💚 Build #24536 succeeded c827ed1bfc7c3c0dd10a08e236b66f11f9bec8ad
  • 💚 Build #24525 succeeded 64ccbf2d8a5d781d8d0a94b10cc7e55643e14a79
  • 💚 Build #24051 succeeded de1a0b5e02ddf0423934ece00ccb45943995c9d4

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

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

History

  • 💔 Build #25187 failed 221b2804aef7766fa75296644a531372f6fe8852
  • 💔 Build #25170 failed af1123e438e40c9a2c4faeae68d6335ea3323710
  • 💔 Build #24899 failed af1123e438e40c9a2c4faeae68d6335ea3323710
  • 💚 Build #24536 succeeded c827ed1bfc7c3c0dd10a08e236b66f11f9bec8ad
  • 💚 Build #24525 succeeded 64ccbf2d8a5d781d8d0a94b10cc7e55643e14a79

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

@dgieselaar dgieselaar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(forgot to submit this comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally you would use the callback form of setState, see https://reactjs.org/docs/react-component.html#setstate

@cauemarcondes cauemarcondes Feb 7, 2020 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dgieselaar even in a function component?

@cauemarcondes

Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@cauemarcondes cauemarcondes changed the title [APM UI] actions menu update [APM] Divide "Actions menu" into sections to improve readability Feb 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to add a marginBottom here, should just use the <SectionSpacer /> component https://github.com/elastic/kibana/blob/master/x-pack/plugins/observability/public/components/action_menu.tsx#L46

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback @phillipb, I added the marginBottom property because I thought the Section component should automatically handle the space between sections for me. Then with CSS I remove the space from the last item.

https://github.com/cauemarcondes/kibana/blob/actions-menu/x-pack/plugins/observability/public/components/action_menu.tsx#L49-L56

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, I see. That makes sense. What do you think about defaulting the marginBottom in the Section component, so others don't need to think about it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They don't have to worry about it, the property is optional if it is missing it will use 0.

@phillipb phillipb Feb 13, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just meant so that they get this behavior by default. This feels like the desired behavior to me, or is this APM specific?

@sorenlouv sorenlouv Feb 13, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the following is more readable:

<Section key={item.key}>
!isLast &&  <SectionSpacer />

Although using css is more performant... 🤔

@sorenlouv sorenlouv Feb 13, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the javascript approach it's clear that we add a space after every section except the last one. That's not possible to see from:

<Section key={item.key} marginBottom={px(units.plus)}>

@phillipb phillipb Feb 13, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't have strong feelings about one approach over the other. I just think if we go with the css approach it should all be handled internally in the Section component. A marginBottom prop feels like an afterthought. Some future person will wonder why it's there and what they should set it to. I'd rather keep the api clean for consistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMO, I expect the Service Component to handle that. Otherwise, what's the benefit that I have by using it? In the end, it just shows the children. Now if I start using it in other parts of the UI, every time I'd have to check if I'm in the last item of the list to not show the space. I agreed with @phillipb that we could add the margin-bottom by default in order to have a clear API. WDYT?

@sorenlouv sorenlouv Feb 14, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we could add the margin-bottom by default in order to have a clear API

👍

@kibanamachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

History

  • 💚 Build #26449 succeeded 65db4d66de550ab928d745c038a921afdd71cd32
  • 💚 Build #25825 succeeded 29071a6f5e04ff8c8651f4082fa7036150f5feb6
  • 💚 Build #25813 succeeded 48c8d52267f1e1398cf74d1d79f3961138a28d50
  • 💚 Build #25477 succeeded 13080464cd3d3f66692cfa95c5f96c76a3248c48
  • 💚 Build #25218 succeeded dfc19f57dc299bfd1569343bf7ef2b9ca26b2af2

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

@cauemarcondes
cauemarcondes merged commit f49581c into elastic:master Feb 17, 2020
@cauemarcondes
cauemarcondes deleted the actions-menu branch February 17, 2020 12:05
sorenlouv added a commit that referenced this pull request Feb 18, 2020
) (#57907)

* transaction actions menu

* transaction actions menu

* fixing pr comments

* fixing pr comments

* fixing pr comments

* fixing pr comments

* fixing unit test

* fixing unit test

* using moment to calculate the timestamp

* renaming labels

* Changing section subtitle

* fixing unit tests

* replacing div for react fragment

* refactoring

* removing marginbottom property

* div is needed to remove the margin from the correct element

Co-authored-by: Cauê Marcondes <55978943+cauemarcondes@users.noreply.github.com>
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
…stic#56623)

* transaction actions menu

* transaction actions menu

* fixing pr comments

* fixing pr comments

* fixing pr comments

* fixing pr comments

* fixing unit test

* fixing unit test

* using moment to calculate the timestamp

* renaming labels

* Changing section subtitle

* fixing unit tests

* replacing div for react fragment

* refactoring

* removing marginbottom property

* div is needed to remove the margin from the correct element
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[APM] Divide "Actions menu" into sections to improve readability

9 participants